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

Incorrect mul foo, undef -> shl foo, undef #46477

Closed
nunoplopes opened this issue Aug 12, 2020 · 7 comments
Closed

Incorrect mul foo, undef -> shl foo, undef #46477

nunoplopes opened this issue Aug 12, 2020 · 7 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla miscompilation

Comments

@nunoplopes
Copy link
Member

Bugzilla Link 47133
Resolution FIXED
Resolved on Oct 23, 2020 18:33
Version trunk
OS All
Blocks #47292 #46070
CC @efriedma-quic,@zmodem,@LebedevRI,@RKSimon,@regehr
Fixed by commit(s) 12d93a2

Extended Description

This is a recent regression in Transforms/InstCombine/mul.ll.
shl foo, undef is poison, so we can't introduce that.

define <2 x i32> @​mulsub1_vec_nonuniform_undef(<2 x i32> %a0, <2 x i32> %a1) {
%sub = sub <2 x i32> %a1, %a0
%mul = mul <2 x i32> %sub, { 4294967292, undef }
ret <2 x i32> %mul
}
=>
define <2 x i32> @​mulsub1_vec_nonuniform_undef(<2 x i32> %a0, <2 x i32> %a1) {
%sub.neg = sub <2 x i32> %a0, %a1
%mul = shl <2 x i32> %sub.neg, { 2, undef }
ret <2 x i32> %mul
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

Example:
<2 x i32> %a0 = < undef, undef >
<2 x i32> %a1 = < undef, undef >

Source:
<2 x i32> %sub = < undef, undef >
<2 x i32> %mul = < #x00000000 (0), #x00000000 (0) >

Target:
<2 x i32> %sub.neg = < #x00000000 (0), #x00000000 (0) >
<2 x i32> %mul = < #x00000000 (0), poison >
Source value: < #x00000000 (0), #x00000000 (0) >
Target value: < #x00000000 (0), poison >

Probably caused by 0c1c756

https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=b697cc49b7cc411c&test=Transforms%2FInstCombine%2Fmul.ll

@nunoplopes
Copy link
Member Author

assigned to @LebedevRI

@LebedevRI
Copy link
Member

Thanks.
This also exposes another issue.
log2base(iN undef) is currently computed as iN undef,
which is obviously incorrect, because log2base(iN %x) u< N,
and undef could be N.

@efriedma-quic
Copy link
Collaborator

The relevant transform is on the following, I think.

define <2 x i32> @​f(<2 x i32> %a0, <2 x i32> %a1) {
%sub = sub <2 x i32> %a1, %a0
%mul = mul <2 x i32> %sub, < i32 4, i32 undef >
ret <2 x i32> %mul
}

instcombine has been doing this since 7.0.

@LebedevRI
Copy link
Member

The relevant transform is on the following, I think.

define <2 x i32> @​f(<2 x i32> %a0, <2 x i32> %a1) {
%sub = sub <2 x i32> %a1, %a0
%mul = mul <2 x i32> %sub, < i32 4, i32 undef >
ret <2 x i32> %mul
}

instcombine has been doing this since 7.0.

Yes, this is not a new miscompile per-se, this will need backporting.

@LebedevRI
Copy link
Member

Added sanitization in 12d93a2.
Hans, i'm not sure about test coverage, but this should be cherry-picked.

Whoever is interested, i've added some FIXME for followup improvements
(not correctness issues):

[NFC][InstCombine] Add FIXME's for getLogBase2() / visitUDivOperand()

These are not correctness issues.

In visitUDivOperand(), if the (potential) divisor is undef, then udiv is
already UB, so it is not incorrect to keep undef as shift amount.

But, that is suboptimal.
We could instead simply drop that select, picking the other operand.

Afterwards, getLogBase2() could assert that there is no undef in divisor.

@zmodem
Copy link
Collaborator

zmodem commented Aug 18, 2020

Pushed to 11.x as 522eeb6 except the llvm/test/Transforms/InstCombine/mul.ll changes because those tests don't exist on the branch.

@aqjune
Copy link
Contributor

aqjune commented Nov 27, 2021

mentioned in issue #47292

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 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 miscompilation
Projects
None yet
Development

No branches or pull requests

5 participants