``` $ cat src.ll define void @src(<2 x i32> %input) { %cond = icmp eq <2 x i32> %input, zeroinitializer %.14 = extractelement <2 x i32> %input, i32 1 %.15 = insertelement <2 x i32> poison, i32 %.14, i32 0 %.16 = extractelement <2 x i32> %input, i32 0 %.17 = insertelement <2 x i32> %.15, i32 %.16, i32 1 %.18 = select <2 x i1> %cond, <2 x i32> %.17, <2 x i32> %input %x = extractelement <2 x i32> %.18, i32 0 %y = extractelement <2 x i32> %.18, i32 1 call void @f(i32 %x, i32 %y) ret void } declare void @f(i32, i32) $ opt -instcombine -S -o - src.ll define void @src(<2 x i32> %input) { %x = extractelement <2 x i32> %input, i32 0 %y = extractelement <2 x i32> %input, i32 1 call void @f(i32 %x, i32 %y) ret void } declare void @f(i32, i32) ``` This is incorrect: https://alive2.llvm.org/ce/z/8X-4kR If input is <0, 2>, (%x, %y) is (2, 2) in src, but it becomes (0, 2) in tgt.
I'll look at this. Reduced (don't need the initial insert/extract): define void @src(<2 x i32> %x) { %r = shufflevector <2 x i32> %x, <2 x i32> undef, <2 x i32> <i32 1, i32 0> %cond = icmp eq <2 x i32> %x, zeroinitializer %s = select <2 x i1> %cond, <2 x i32> %r, <2 x i32> %x %s0 = extractelement <2 x i32> %s, i32 0 %s1 = extractelement <2 x i32> %s, i32 1 call void @f(i32 %s0, i32 %s1) ret void }
We already had a regression test that Alive2 flags as a potential miscompile. I think we just overlooked a basic requirement of select value equivalence substitution: https://reviews.llvm.org/rGc0b0da468490 https://reviews.llvm.org/rGc590a9880d7a Godbolt shows that this would be a new miscompile for release 12.0: https://godbolt.org/z/qqbbxhe6z I'm not sure what changed to make that visible, but let's see if we can still get this into the release. Marking as release blocker.
It would be good to check that the same problem doesn't exist in InstSimplify (see simplifySelectWithICmpCond).
(In reply to Sanjay Patel from comment #2) > I'm not sure what changed to make that visible, but let's see if we can > still get this into the release. Marking as release blocker. Aha - I'm losing track of reviews: https://reviews.llvm.org/D96945 If that change is required to expose the bug, then it's not in the 12.0 release AFAIK.
(In reply to Nikita Popov from comment #3) > It would be good to check that the same problem doesn't exist in > InstSimplify (see simplifySelectWithICmpCond). Yes, good point. Looks like we can still hit the bug there too. https://alive2.llvm.org/ce/z/HGETc3 Not sure if it's just me, but the online instance of Alive2 is acting strange, so for reference that should show: define <2 x i32> @src(<2 x i32> %x) { %0: %x10 = shufflevector <2 x i32> %x, <2 x i32> undef, 1, 0 %cond = icmp eq <2 x i32> %x, { 0, 0 } %s = select <2 x i1> %cond, <2 x i32> %x10, <2 x i32> { 0, 0 } ret <2 x i32> %s } => define <2 x i32> @tgt(<2 x i32> %x) { %0: ret <2 x i32> { 0, 0 } } Transformation doesn't verify! ERROR: Value mismatch Example: <2 x i32> %x = < #x00000000 (0), #xffffffff (4294967295, -1) > Source: <2 x i32> %x10 = < #xffffffff (4294967295, -1), #x00000000 (0) > <2 x i1> %cond = < #x1 (1), #x0 (0) > <2 x i32> %s = < #xffffffff (4294967295, -1), #x00000000 (0) > Target: Source value: < #xffffffff (4294967295, -1), #x00000000 (0) > Target value: < #x00000000 (0), #x00000000 (0) >
InstSimplify patches: https://reviews.llvm.org/rG78e5cf66fec5 https://reviews.llvm.org/rGe2a0f512eaca
Thank you for the swift fix! I also see that online Alive2 is a bit strange; since I am not familiar with the interaction between online Alive2 and the Alive2 binary, I have no idea what's happening.. I shared the issue with other Alive2 people. Whenever there is a problem, feel free to send us (Nuno,John,me) mails. You can leave a thread at Alive2 repo's discussion forum as well. :)
So all 4 commits listed in "Fixed By Commits" need to be backported?
(In reply to Tom Stellard from comment #8) > So all 4 commits listed in "Fixed By Commits" need to be backported? Yes, please. Technically, we could fix this bug with the 1st two (instcombine) patches alone. But backporting all 4 (two patches are just adding tests / NFC) would be the best way to be sure we don't hit the problem in any code.
(In reply to Sanjay Patel from comment #9) > (In reply to Tom Stellard from comment #8) > > So all 4 commits listed in "Fixed By Commits" need to be backported? > > Yes, please. > > Technically, we could fix this bug with the 1st two (instcombine) patches > alone. But backporting all 4 (two patches are just adding tests / NFC) would > be the best way to be sure we don't hit the problem in any code. Can you double check that the list of commits is correct. 2a0f512eaca does not exist in the git tree.
(In reply to Tom Stellard from comment #10) > Can you double check that the list of commits is correct. 2a0f512eaca does > not exist in the git tree. Sorry lost a digit - that should be: e2a0f512eaca
Looks like 78e5cf66fec5 and e2a0f512eaca were cherry-picked to 12.x, and I'm now seeing llvm/test/Transforms/InstSimplify/select.ll fail on that branch: /data/users/smeenai/llvm-project/llvm/test/Transforms/InstSimplify/select.ll:974:15: error: CHECK-NEXT: is not on the line after the previous match ; CHECK-NEXT: [[T1:%.*]] = call i32 @llvm.ctpop.i32(i32 [[X:%.*]]) ^ <stdin>:457:2: note: 'next' match was here %t1 = call i32 @llvm.ctpop.i32(i32 %x) ^ <stdin>:455:31: note: previous match ended here define i32 @select_ctpop_zero(i32 %x) { ^ <stdin>:456:1: note: non-matching line after previous match is here %t0 = icmp eq i32 %x, 0 ^
(In reply to Shoaib Meenai from comment #12) > Looks like 78e5cf66fec5 and e2a0f512eaca were cherry-picked to 12.x, and I'm > now seeing llvm/test/Transforms/InstSimplify/select.ll fail on that branch: Yes - this is my fault for letting a formatting change slip into the commit. I think the cherry-pick is over-reaching as described here (we picked up an extra test for the branch unintentionally): https://github.com/llvm/llvm-project/commit/266c82f94da232d
I committed a patch that should fix this: https://github.com/llvm/llvm-project/commit/2db5d42193abefcb41b10bd70b7ab536cb03f1cc
Merged: 8e2ff387d30d