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

[Danila Kutenin via Phabricator via llvm-commits]

danlark added inline comments.


================
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");
----------------
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?


================
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();
----------------
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()**


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71568