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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


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


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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1761
+
+  // (rem (mul nuw/nsw X, Y), (mul {nsw} X, Z))
+  //      if Y >= Z
----------------
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?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1767-1769
+    BO->setHasNoSignedWrap();
+    if (BO0HasNUW)
+      BO->setHasNoUnsignedWrap();
----------------
nit: It is easier to write:

  BO->setHasNoSignedWrap(BO0HasNSW);
  BO->setHasNoUnsignedWrap(BO0HasNUW);


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1705
+  ConstantInt *ConstY = GetOperandAsConstantInt(Y);
+  ConstantInt *ConstZ = GetOperandAsConstantInt(Z);
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014