Incorrect mul foo, undef -> shl foo, undef

[nunoplopes]

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]

assigned to @LebedevRI

[LebedevRI]

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]

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]

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]

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]

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]

mentioned in issue #47292