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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148609