[PATCH] D143014: Add constant combines for `(urem/srem (mul X, Y), (mul X, Z))`

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1739-1742
+    if (IsSRem || BO0HasNSW)
+      BO->setHasNoSignedWrap();
+    if (!IsSRem || BO0HasNUW)
+      BO->setHasNoUnsignedWrap();
----------------
nit:
  BO->setHasNoSignedWrap(IsSRem || BO0HasNSW);
  BO->setHasNoUnsignedWrap(!IsSRem || BO0HasNUW);


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1738
+  APInt RemYZ = IsSRem ? APIntY.srem(APIntZ) : APIntY.urem(APIntZ);
+  // (rem (mul nuw/nsw X, Y), (mul X, Z))
+  //      if (rem Y, Z) == 0
----------------
goldstein.w.n wrote:
> sdesmalen wrote:
> > There is an implied condition that if `RemYZ = 0`, then `Y >= Z`, consequently `mul X, Z` cannot overflow if `mul X, Y` cannot overflow.
> > 
> > 
> > I think the code would be easier to follow if you do something like this:
> > 
> >   bool LHSNoWrap = IsSRem ? BO0HasNSW : BO0HasNUW;
> >   bool RHSNoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
> >   if (Y >= Z && LHSNoWrap) {
> >     // Handle case for RemYZ == 0 and Y >= Z
> >   } else if (Y < Z && RHSNoWrap) {
> >     // Handle case for RemYZ == Y
> >   }
> > There is an implied condition that if `RemYZ = 0`, then `Y >= Z`, consequently `mul X, Z` cannot overflow if `mul X, Y` cannot overflow.
> > 
> the `Y >= Z` -> implied flags doesn't seem to verify in all cases i.e:
> https://alive2.llvm.org/ce/z/bc8BXG
> vs
> https://alive2.llvm.org/ce/z/uERkKH
> 
> or
> https://alive2.llvm.org/ce/z/BUuMfn
> vs
> https://alive2.llvm.org/ce/z/X6ZEtQ
> 
> I don't doubt that there may be more cases we can add, but would prefer to leave that as a todo.
> > 
> > I think the code would be easier to follow if you do something like this:
> > 
> >   bool LHSNoWrap = IsSRem ? BO0HasNSW : BO0HasNUW;
> >   bool RHSNoWrap = IsSRem ? BO1HasNSW : BO1HasNUW;
> >   if (Y >= Z && LHSNoWrap) {
> >     // Handle case for RemYZ == 0 and Y >= Z
> >   } else if (Y < Z && RHSNoWrap) {
> >     // Handle case for RemYZ == Y
> >   }
> 
> Done.
That makes sense, my thinking here was guided by the unsigned case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014