[PATCH] D148609: [InstCombine] Fix buggy `(mul X, Y)` -> `(shl X, Log2(Y))` transform PR62175

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1220-1226
+    bool Usable = AssumeNonZero;
+    if (!AssumeNonZero) {
+      BinaryOperator *BO = cast<BinaryOperator>(Op);
+      // nuw will be set if the `shl` is trivially non-zero.
+      Usable = BO->hasNoUnsignedWrap() || BO->hasNoSignedWrap();
+    }
+    if (Usable)
----------------
Would be a bit cleaner imho.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1252
+      if (Value *LogY = takeLog2(Builder, MinMax->getRHS(), Depth,
+                                 AssumeNonZero, DoFold))
         return IfFold([&]() {
----------------
This isn't right: The result being non-zero doesn't imply the arguments being non-zero here, see  https://alive2.llvm.org/ce/z/AtZmi_. For the sake of simplicity, just always pass AssumeNonZero=false here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148609