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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n added a comment.

Sorry for the delay, didn't see your replies till just now :(



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1723
+
+  bool IsSRem = I.getOpcode() == Instruction::SRem;
+
----------------
sdesmalen wrote:
> nit: Please move this closer to its use. Also, is it worth calling it `IsSigned`?
> nit: Please move this closer to its use. Also, is it worth calling it `IsSigned`?

Moved closer, it was original `IsSigned` but matt asked for change to `IsSRem`. Personally I'm agnostic.


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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1761
+
+  // (rem (mul nuw/nsw X, Y), (mul {nsw} X, Z))
+  //      if Y >= Z
----------------
sdesmalen wrote:
> if `Y >= Z` and `mul X, Y` doesn't overflow, then `mul X, Z` also can't overflow, so the test for B01HasNSW can be removed?
> if `Y >= Z` and `mul X, Y` doesn't overflow, then `mul X, Z` also can't overflow, so the test for B01HasNSW can be removed?

Doesn't verify: https://alive2.llvm.org/ce/z/_yqMCk


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1705
+  ConstantInt *ConstY = GetOperandAsConstantInt(Y);
+  ConstantInt *ConstZ = GetOperandAsConstantInt(Z);
+
----------------
sdesmalen wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > Why doesn't this use `match(Y, m_APInt(APIntY))` etc? As far as I can tell you don't use the ConstantInt itself, and m_APInt already handles splats.
> > > Why doesn't this use `match(Y, m_APInt(APIntY))` etc? As far as I can tell you don't use the ConstantInt itself, and m_APInt already handles splats.
> > 
> > That would work for this patch (can update if thats preference), but last in the series (D143417) generalizes this to non-constants so would have to return to this code either way.
> Since we're reviewing this patch, and not  D143417, I would prefer you to use `m_APInt` here. D143417 can change it back if that's what it needs to do.
DOne, likewise for the shift patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014