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

InstCombine incorrectly folds select of vector #49176

Closed
aqjune opened this issue Apr 4, 2021 · 16 comments
Closed

InstCombine incorrectly folds select of vector #49176

aqjune opened this issue Apr 4, 2021 · 16 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla

Comments

@aqjune
Copy link
Contributor

aqjune commented Apr 4, 2021

Bugzilla Link 49832
Resolution FIXED
Resolved on May 29, 2021 08:41
Version trunk
OS All
Blocks #48246 #48661
CC @RKSimon,@nikic,@smeenai,@rotateright,@tstellar
Fixed by commit(s) c0b0da4 c590a98 78e5cf6 e2a0f51 c89d500 4a12f51 266c82f 8e2ff38

Extended Description

$ cat src.ll
define void @&#8203;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 @&#8203;f(i32 %x, i32 %y)
    ret void
}

declare void @&#8203;f(i32, i32)
$ opt -instcombine -S -o - src.ll
define void @&#8203;src(<2 x i32> %input) {
  %x = extractelement <2 x i32> %input, i32 0
  %y = extractelement <2 x i32> %input, i32 1
  call void @&#8203;f(i32 %x, i32 %y)
  ret void
}

declare void @&#8203;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.

@aqjune
Copy link
Contributor Author

aqjune commented Apr 4, 2021

assigned to @rotateright

@rotateright
Copy link
Contributor

rotateright commented Apr 5, 2021

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
}

@rotateright
Copy link
Contributor

rotateright commented Apr 5, 2021

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.

@nikic
Copy link
Contributor

nikic commented Apr 5, 2021

It would be good to check that the same problem doesn't exist in InstSimplify (see simplifySelectWithICmpCond).

@rotateright
Copy link
Contributor

rotateright commented Apr 5, 2021

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.

@rotateright
Copy link
Contributor

rotateright commented Apr 5, 2021

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) >

@rotateright
Copy link
Contributor

rotateright commented Apr 5, 2021

@aqjune
Copy link
Contributor Author

aqjune commented Apr 6, 2021

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. :)

@tstellar
Copy link
Collaborator

tstellar commented May 1, 2021

So all 4 commits listed in "Fixed By Commits" need to be backported?

@rotateright
Copy link
Contributor

rotateright commented May 3, 2021

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.

@tstellar
Copy link
Collaborator

tstellar commented May 3, 2021

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.

@rotateright
Copy link
Contributor

rotateright commented May 3, 2021

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:
e2a0f51

@smeenai
Copy link
Collaborator

smeenai commented May 10, 2021

Looks like 78e5cf6 and e2a0f51 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:%.]])
^
:457:2: note: 'next' match was here
%t1 = call i32 @​llvm.ctpop.i32(i32 %x)
^
:455:31: note: previous match ended here
define i32 @​select_ctpop_zero(i32 %x) {
^
:456:1: note: non-matching line after previous match is here
%t0 = icmp eq i32 %x, 0
^

@rotateright
Copy link
Contributor

rotateright commented May 10, 2021

Looks like 78e5cf6 and e2a0f51 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):
266c82f94da232d

@tstellar
Copy link
Collaborator

tstellar commented May 10, 2021

I committed a patch that should fix this:
2db5d42

@tstellar
Copy link
Collaborator

tstellar commented May 10, 2021

Merged: 8e2ff38

@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

5 participants