[PATCH] D144225: [InstCombine] Add constant combines for `(urem/srem (shl X, Y), (shl X, Z))`

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1707
+  bool ShiftX = false, ShiftY = false, ShiftZ = false;
+  if ((match(Op0, m_Mul(m_Value(X), m_APInt(Y))) &&
+       match(Op1, m_c_Mul(m_Specific(X), m_APInt(Z)))) ||
----------------
It might be a bit easier to follow, if you explicitly do the scaling while doing the matching, i.e.

  APInt Y, Z;
  const APInt *MatchY = nullptr, *MatchZ = nullptr;

  // Match and normalise shift-amounts to multiplications
  if (match(Op0, m_c_Mul(m_Value(X), m_APInt(MatchY)))) {
    Y = *MatchY;
    if (match(Op1, m_Shl(m_Specific(X), m_APInt(MatchZ))))
      //  rem(mul(x, y), shl(x, z))
      Z = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchZ);
    else if (match(Op1, m_c_Mul(m_Specific(X), m_APInt(MatchZ))))
      //  rem(mul(x, y), mul(x, z))
      Z = *MatchZ;
  } else if (match(Op0, m_Shl(m_Value(X), m_APInt(MatchY)))) {
    Y = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchY);
    if (match(Op1, m_Shl(m_Specific(X), m_APInt(MatchZ))))
      //  rem(shl(x, y), shl(x, z))
      Z = APInt(X->getType()->getScalarSizeInBits(), 1).shl(*MatchZ);
    else if (match(Op1, m_c_Mul(m_Specific(X), m_APInt(MatchZ))))
      //  rem(shl(x, y), mul(x, z))
      Z = *MatchZ;
  }

  if (!MatchY || !MatchZ)
    return nullptr;


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1722-1724
+  } else if (match(Op0, m_Shl(m_APInt(Y), m_Value(X))) &&
+             match(Op1, m_Shl(m_APInt(Z), m_Specific(X)))) {
+    ShiftX = true;
----------------
I can't see this case being tested anywhere. I'd suggest singling this case out from the other logic and handling it separately. Maybe you can move that to another patch (or make it part of D143417, which tries to handle a similar case). That makes this patch a bit simpler and avoids the need for `GetBinOpOut`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144225