[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:
> > goldstein.w.n wrote:
> > > 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.
> > No strong opinion either way. The conflict resolution here looks pretty trivial.
> > No strong opinion either way. The conflict resolution here looks pretty trivial.
> 
> Okay, I'll push as is.
> 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.

Created issue: https://github.com/llvm/llvm-project/issues/62221


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148609