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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for simplifying this patch a bit!



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1709
+
+  auto GetOperandAsConstantInt = [&](Value *Op) -> ConstantInt * {
+    if (I.getType()->isVectorTy()) {
----------------
No variables really need to be captured for this function. You can also write it more compactly as:

  if (Op->getType()->isVectorTy())
    if (auto *COp = dyn_cast<Constant>(Op))
      return dyn_cast<ConstantInt>(COp->getSplatValue());
  return dyn_cast<ConstantInt>(Op);




================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1695-1696
+  Value *X = nullptr, *Y, *Z;
+  // Do this by hand as opposed to using m_Specific because either A/B (or
+  // C/D) can be our X.
+  if (A == C || A == D) {
----------------
goldstein.w.n wrote:
> sdesmalen wrote:
> > Is that true? `shl` is not commutative, i.e. `(a * b) == (b * a)`, but `(a << b) != (b << a)` 
> > 
> > Also, this patch is already quite complicated, I would prefer to keep it simple at first and only match: `urem/srem (mul/shl X, Y), (mul/shl X, Z)`
> > where 'X' is matched with m_Specific. Then later patches can maybe relax those conditions further.
> > Is that true? `shl` is not commutative, i.e. `(a * b) == (b * a)`, but `(a << b) != (b << a)` 
> >
> 
> You are right. Made this patch only cover `mul` so its a non-issue here, but the `shl` patch (see D144225) cover it.
>  
> > Also, this patch is already quite complicated, I would prefer to keep it simple at first and only match: `urem/srem (mul/shl X, Y), (mul/shl X, Z)`
> > where 'X' is matched with m_Specific. Then later patches can maybe relax those conditions further.
> 
> I tried doing this, but found it significantly more complicated because the `m_Specific` logic for both lhs/rhs as well as special cases for `shl` paired with `mul`. To try and simplify I spit off the `shl` patch (so hopefully easier to review). If you insist, however, it is implementable to `match` + `m_Specific` so LMK.
To use `m_Specific` you can do the following:

  Value *X = nullptr, *Y = nullptr, *Z = nullptr;
  if (!match(Op0, m_Mul(m_Value(X), m_Value(Y))) &&
      !match(Op0, m_Shl(m_Value(X), m_Value(Y))))
    return nullptr;
  if (!match(Op1, m_Mul(m_Specific(X), m_Value(Z))) &&
      !match(Op1, m_Shl(m_Specific(X), m_Value(Z))))
    return nullptr;

I prefer this explicit matching over trying assign X, Y and Z from A, B, C and D, since that logic tries to cover various different combinations and is harder to follow. Note that the version I wrote above also allows for the case where `Y` and `Z` are a `Shl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014