[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) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014