[PATCH] D147108: [InstCombine] Add transforms for `(rem (shl Y, X), (shl Z, X))`

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1762
 
 // Variety of transform for (urem/srem (mul/shl X, Y), (mul/shl X, Z))
 static Instruction *simplifyIRemMulShl(BinaryOperator &I,
----------------
Can you add this new case to the description here?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1766
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1), *X = nullptr;
+  APInt const *MatchY, *MatchZ;
   APInt Y, Z;
----------------
I wonder if you can do something similar to `MatchShiftOrMul`, so that we use a similar mechanism

  if (MatchShiftOrMul(Op0, X, Y) &&
      MatchShiftOrMul(Op1, X, Z, /*MatchSpecific=*/true))
    ; // Do nothing
  else if (MatchShiftConstByVal(Op0, Y, X) &&
           MatchShiftConstByVal(Op1, Z, X, /*MatchSpecific=*/true))
    ShiftX = true;
  else
    return nullptr;

Just note that this requires changing the logic in `MatchShiftOrMul` which currently checks if `V` is nullptr to use `m_Specific` only when requested through an operand to the function. This way, you don't need to 'reset' `V` in between the two calls. I'm actually thinking that this explicit behaviour may be more desirable anyway, because then there is no implicit dependence between successive calls of this function.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1809
 
+  auto GetBinOpOut = [&](Value *RemSimplification) -> BinaryOperator * {
+    return ShiftX ? BinaryOperator::CreateShl(RemSimplification, X)
----------------
Can you add a comment describing the need for this function?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1809
 
+  auto GetBinOpOut = [&](Value *RemSimplification) -> BinaryOperator * {
+    return ShiftX ? BinaryOperator::CreateShl(RemSimplification, X)
----------------
sdesmalen wrote:
> Can you add a comment describing the need for this function?
nit: is it worth renaming this to `CreateMulOrShift` ?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1810
+  auto GetBinOpOut = [&](Value *RemSimplification) -> BinaryOperator * {
+    return ShiftX ? BinaryOperator::CreateShl(RemSimplification, X)
+                  : BinaryOperator::CreateMul(X, RemSimplification);
----------------
nit: rename to `ShiftByX` ?

This might be overkill because there are only two options, but for readability purposes I wondered if it was worth adding an enum which describes the two options, e.g. `enum RemMatchKind { NoMatch = 0, MulValByConst = 1, ConstShiftedByVal = 2 };` or something.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1818-1820
   // (rem (mul X, Y), (mul nuw/nsw X, Z))
   //      if (rem Y, Z) == Y
   //          -> (mul nuw/nsw X, Y)
----------------
A number of these comments are now out of date. You could add a comment which describes that:

  C << X <=> C * (1 << X)

so that the same logic here still applies when you conceptually substitute `(1 << X)` by `X`.
It just requires that the return value must be implemented differently (which is why there is the `GetBinOpOut` function)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1822
   if (RemYZ == Y && BO1NoWrap) {
-    BinaryOperator *BO =
-        BinaryOperator::CreateMul(X, ConstantInt::get(I.getType(), Y));
+    BinaryOperator *BO = GetBinOpOut(ConstantInt::get(I.getType(), Y));
     // Copy any overflow flags from Op0.
----------------
Can you pass in the APInt instead, and do the `ConstantInt::get(...)` in `GetBinOptOut`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147108