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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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:
> > 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.
> I believe for now it is correct, C1 is a logarithm of Op1, if Op1 is INT_MIN, then it should not have nsw. So, if all are not int_min, it will be propagated, otherwise, it will not.
> 
> ```
> define i32 @shift_if_power2_nuw_nsw_min(i32 %x, i1 %cond) {
>   %sel = select i1 %cond, i32 2, i32 -2147483648
>   %r = mul nuw nsw i32 %sel, %x
>   ret i32 %r
> }
> 
> ===>
> 
> define i32 @shift_if_power2_nuw_nsw_min(i32 %x, i1 %cond) {
>   %sel = select i1 %cond, i32 1, i32 31
>   %r =  shl nuw i32 i32 %x, %sel
>   ret i32 %r
> }
> ```
> 
Can you point me to the (vector) test that shows it working as expected?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:116
     if (isa<UndefValue>(Elt)) {
-      Elts.push_back(UndefValue::get(Ty->getScalarType()));
+      Elts.push_back(ConstantInt::get(Ty->getScalarType(), 0));
       continue;
----------------
You really want to only use `Constant::replaceUndefsWith()` to fixup that single case,
not pessimize everything that uses this generic utility..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71568