Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

miscompile due to instcombine #50693

Closed
regehr opened this issue Aug 5, 2021 · 4 comments
Closed

miscompile due to instcombine #50693

regehr opened this issue Aug 5, 2021 · 4 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@regehr
Copy link
Contributor

regehr commented Aug 5, 2021

Bugzilla Link 51351
Resolution FIXED
Resolved on Sep 29, 2021 10:09
Version trunk
OS Linux
CC @LebedevRI,@RKSimon,@nikic,@nunoplopes,@rotateright
Fixed by commit(s) ea56dcb 4414e2a

Extended Description

this was found by my student Yuyou Fan

regehr@home:~/tmp$ cat reduced.ll
; ModuleID = 'foo.ll'
source_filename = "foo.ll"

define i32 @​src(i64 %x, i32 %nbits) local_unnamed_addr {
%t0 = zext i32 %nbits to i64
%t1 = shl i64 -1, %t0
%t2 = ashr exact i64 %t1, %t0
%t3 = add i32 %nbits, -33
%t4 = and i64 %t2, %x
%t5 = trunc i64 %t4 to i32
%t6 = shl i32 %t5, %t3
ret i32 %t6
}
regehr@home:/tmp$ opt -instcombine reduced.ll -o reduced-opt.ll
regehr@home:
/tmp$ ~/alive2/build/alive-tv reduced.ll reduced-opt.ll


define i32 @​src(i64 %x, i32 %nbits) {
%0:
%t0 = zext i32 %nbits to i64
%t1 = shl i64 -1, %t0
%t2 = ashr exact i64 %t1, %t0
%t3 = add i32 %nbits, 4294967263
%t4 = and i64 %t2, %x
%t5 = trunc i64 %t4 to i32
%t6 = shl i32 %t5, %t3
ret i32 %t6
}
=>
define i32 @​src(i64 %x, i32 %nbits) {
%0:
%t3 = add i32 %nbits, 4294967263
%1 = trunc i64 %x to i32
%2 = shl i32 %1, %t3
%t6 = and i32 %2, 2147483647
ret i32 %t6
}
Transformation doesn't verify!

ERROR: Value mismatch

Example:
i64 %x = #x0000000040000000 (1073741824)
i32 %nbits = #x00000022 (34)

Source:
i64 %t0 = #x0000000000000022 (34)
i64 %t1 = #xfffffffc00000000 (18446744056529682432, -17179869184)
i64 %t2 = #xffffffffffffffff (18446744073709551615, -1)
i32 %t3 = #x00000001 (1)
i64 %t4 = #x0000000040000000 (1073741824)
i32 %t5 = #x40000000 (1073741824)
i32 %t6 = #x80000000 (2147483648, -2147483648)

Target:
i32 %t3 = #x00000001 (1)
i32 %1 = #x40000000 (1073741824)
i32 %2 = #x80000000 (2147483648, -2147483648)
i32 %t6 = #x00000000 (0)
Source value: #x80000000 (2147483648, -2147483648)
Target value: #x00000000 (0)

Summary:
0 correct transformations
1 incorrect transformations
0 failed-to-prove transformations
0 Alive2 errors
regehr@home:~/tmp$

@regehr
Copy link
Contributor Author

regehr commented Aug 5, 2021

oops the "exact" flag is a red herring and is not necessary, I'll teach my llvm-reduce to try to nuke UB flags

@regehr
Copy link
Contributor Author

regehr commented Aug 5, 2021

(this was verified against tot as of Aug 4 2021)

@rotateright
Copy link
Contributor

We're missing a simplification of the opposite shifts of -1:
https://alive2.llvm.org/ce/z/m5X3PJ

But if we patch that, I suspect we'll cover up a logic bug in dropRedundantMaskingOfLeftShiftInput(). Something in there isn't sufficiently guarded - maybe we need to require 'lshr' in some spot that is accepting any 'shr'?

@rotateright
Copy link
Contributor

Should be fixed with:
https://reviews.llvm.org/rGea56dcb73012

Note - I botched what was supposed to be an NFC add-tests-patch just before that:
https://reviews.llvm.org/rGd3e2067c7c42

So the test diff is misleading.

Also added the missing simplification which should make it harder to get into trouble from patterns like this:
https://reviews.llvm.org/rG4414e2ad97d5

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

2 participants