[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([&]() {
----------------
goldstein.w.n wrote:
> 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?
> 
> > 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?
> 

The rationale being then the patch for Step 2 can just be backported w.o any conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148609