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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1252
+      if (Value *LogY = takeLog2(Builder, MinMax->getRHS(), Depth,
+                                 AssumeNonZero, DoFold))
         return IfFold([&]() {
----------------
nikic wrote:
> 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.
> 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.

Good catch. Thats been buggy since llvm15: https://godbolt.org/z/ehen9hz6G
We probably need to backport?

Should I do the following so backporting is easier?

1) Revert D146347
2) Add `AssumeNonZero` with `false` in this umin/umax case
3) Repost D146347 with AssumeNonZero changes?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148609