New user self-registration is disabled due to spam. For an account please email bugs-admin@lists.llvm.org with your e-mail address and full name.

Bug 47133 - Incorrect mul foo, undef -> shl foo, undef
Summary: Incorrect mul foo, undef -> shl foo, undef
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: All All
: P normal
Assignee: Roman Lebedev
URL:
Keywords: miscompilation
Depends on:
Blocks: 47948 release-11.0.0
  Show dependency tree
 
Reported: 2020-08-12 04:35 PDT by Nuno Lopes
Modified: 2020-10-23 18:33 PDT (History)
6 users (show)

See Also:
Fixed By Commit(s): 12d93a27e7b78d58dd00817cb737f273d2dba8ae


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nuno Lopes 2020-08-12 04:35:38 PDT
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 https://github.com/llvm/llvm-project/commit/0c1c756a31536666a7b6f5bdb744dbce923a0c9e


https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=b697cc49b7cc411c&test=Transforms%2FInstCombine%2Fmul.ll
Comment 1 Roman Lebedev 2020-08-12 07:49:41 PDT
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`.
Comment 2 Eli Friedman 2020-08-12 10:58:22 PDT
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.
Comment 3 Roman Lebedev 2020-08-12 11:09:51 PDT
(In reply to Eli Friedman from comment #2)
> 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.
Comment 4 Roman Lebedev 2020-08-12 12:09:58 PDT
Added sanitization in 12d93a27e7b78d58dd00817cb737f273d2dba8ae.
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.
Comment 5 Hans Wennborg 2020-08-18 03:27:45 PDT
Pushed to 11.x as 522eeb66edfb0d6c4656d3f7a3f635544def30db except the llvm/test/Transforms/InstCombine/mul.ll changes because those tests don't exist on the branch.