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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
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:
> > 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`.
> > 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;
> > 
> This doesn't quite work. It misses things like:
> ```
> %a = mul i8 %Y, %X
> %b = mul i8 %X, %Z
> ```
> 
> To do (just `mul`) with match it needs to be:
> ```
> if ((match(Op0, m_Mul(m_Value(X), m_Value(Z))) &&
>      match(Op1, m_c_Mul(m_Specific(X), m_Value(Y)))) ||
>     (match(Op0, m_Mul(m_Value(Z), m_Value(X))) &&
>      match(Op1, m_c_Mul(m_Specific(X), m_Value(Y)))))
> ```
> To get `shl` will be 3x more of these (`shl/mul`, `mul/shl`, `shl/shl`).
> > 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`.
> 
> Okay. Will change to use match.
> 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`.

No changes to results in the mul case, but this improves the codegen for the generic mul/shl case as the handwritten logic was just glossing over some handleable cases :)

See D144225 for all the match logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014