[PATCH] D71568: [InstCombine] `select + mul` -> `select + shl` with power of twos.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:267-269
+  Constant *C1 = getLogBase2(Op0->getType(), cast<Constant>(Op1));
+  if (!C1)
+    llvm_unreachable("Failed to constant fold mul -> logbase2");
----------------
danlark wrote:
> lebedev.ri wrote:
> > What happens with undef lanes?
> > (they should become `0`, do not keep them as `undef`.
> > there was some helper for that, can't find it now "replace undef values with")
> They will remain undef as in division, why is 0 better?
Keeping `undef` in this situation is (a very common) miscompilation.
```
define <2 x i8> @shift_if_different_lanes_undef_vector(<2 x i8> %px, i1 %cond) {
%0:
  %sel = select i1 %cond, <2 x i8> { 16, undef }, <2 x i8> { undef, 32 }
  %r = mul <2 x i8> %px, %sel
  ret <2 x i8> %r
}
=>
define <2 x i8> @shift_if_different_lanes_undef_vector(<2 x i8> %px, i1 %cond) {
%0:
  %sel = select i1 %cond, <2 x i8> { 4, undef }, <2 x i8> { undef, 5 }
  %r = shl <2 x i8> %px, %sel
  ret <2 x i8> %r
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

Example:
<2 x i8> %px = < undef, undef >
i1 %cond = undef

Source:
<2 x i8> %sel = < undef, undef >
<2 x i8> %r = < undef, undef >

Target:
<2 x i8> %sel = < #x04 (4), #x08 (8) >
<2 x i8> %r = < #x00 (0)        [based on undef value], poison >
Source value: < undef, undef >
Target value: < #x00 (0), poison >
```
```
define <2 x i8> @shift_if_different_lanes_undef_vector(<2 x i8> %px, i1 %cond) {
%0:
  %sel = select i1 %cond, <2 x i8> { 16, undef }, <2 x i8> { undef, 32 }
  %r = mul <2 x i8> %px, %sel
  ret <2 x i8> %r
}
=>
define <2 x i8> @shift_if_different_lanes_undef_vector(<2 x i8> %px, i1 %cond) {
%0:
  %sel = select i1 %cond, <2 x i8> { 4, 0 }, <2 x i8> { 0, 5 }
  %r = shl <2 x i8> %px, %sel
  ret <2 x i8> %r
}
Transformation seems to be correct!
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:274-275
+  if (I.hasNoSignedWrap()) {
+    const APInt *V;
+    if (match(C1, m_APInt(V)) && *V != V->getBitWidth() - 1)
+      Shl->setHasNoSignedWrap();
----------------
danlark wrote:
> lebedev.ri wrote:
> > This isn't sound.
> > We want to not propagate `NSW` if *any* lane matches this predicate, not just if they all do..
> > So you want
> > ```
> > if (I.hasNoSignedWrap() && !C1.isNotMinSignedValue())
> >   Shl->setHasNoSignedWrap();
> > ```
> Yeah, I agree. Maybe you meant !cast<Constant>(Op1)->**isMinSignedValue()**
I'm very sure i didn't - that has the same problem of only checking for splat INT_MIN while we want to check if any channel is INT_MIN.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71568/new/

https://reviews.llvm.org/D71568