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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
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:
> 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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1701
+    Z = A == C ? D : C;
+  } else if (B == C || B == D) {
+    X = B;
----------------
sdesmalen wrote:
> sdesmalen wrote:
> > When I comment this part of the condition out, all tests still pass.
> When I comment this part of the condition out, all tests still pass
> When I comment this part of the condition out, all tests still pass

Added more tests that require this logic.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1701
+    Z = A == C ? D : C;
+  } else if (B == C || B == D) {
+    X = B;
----------------
goldstein.w.n wrote:
> sdesmalen wrote:
> > sdesmalen wrote:
> > > When I comment this part of the condition out, all tests still pass.
> > When I comment this part of the condition out, all tests still pass
> > When I comment this part of the condition out, all tests still pass
> 
> Added more tests that require this logic.
> When I comment this part of the condition out, all tests still pass.

Removed from this patch.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1716-1718
+  auto CX = dyn_cast<Constant>(X);
+  if (CX && CX->isOneValue())
+    return nullptr;
----------------
sdesmalen wrote:
> When I comment this code out, all tests still pass.
> 
> Can this case ever happen? (I would have expected InstCombine/InstSimplify to have folded that case away already)
> When I comment this code out, all tests still pass.
> 
> Can this case ever happen? (I would have expected InstCombine/InstSimplify to have folded that case away already)

Removed the BO0/BO1 checks, but !X can happen.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1720-1733
+  auto ConstY = dyn_cast<ConstantInt>(Y);
+  auto ConstZ = dyn_cast<ConstantInt>(Z);
+  if (I.getType()->isVectorTy()) {
+    auto VConstY = dyn_cast<Constant>(Y);
+    auto VConstZ = dyn_cast<Constant>(Z);
+    if (VConstY && VConstZ) {
+      VConstY = VConstY->getSplatValue();
----------------
sdesmalen wrote:
> nit: You can write this a bit shorter like this:
> 
>   ConstantInt *ConstY = nullptr, *ConstZ = nullptr;
>   if (I.getType()->isVectorTy()) {
>     if (auto *CY = dyn_cast<Constant>(Y))
>       ConstY = dyn_cast<ConstantInt>(CY->getSplatValue());
>     if (auto *CZ = dyn_cast<Constant>(Z))
>       ConstZ = dyn_cast<ConstantInt>(CZ->getSplatValue());
>   } else {
>     ConstY = dyn_cast<ConstantInt>(Y);
>     ConstZ = dyn_cast<ConstantInt>(Z);
>   } 
> 
> nit: You can write this a bit shorter like this:
> 
>   ConstantInt *ConstY = nullptr, *ConstZ = nullptr;
>   if (I.getType()->isVectorTy()) {
>     if (auto *CY = dyn_cast<Constant>(Y))
>       ConstY = dyn_cast<ConstantInt>(CY->getSplatValue());
>     if (auto *CZ = dyn_cast<Constant>(Z))
>       ConstZ = dyn_cast<ConstantInt>(CZ->getSplatValue());
>   } else {
>     ConstY = dyn_cast<ConstantInt>(Y);
>     ConstZ = dyn_cast<ConstantInt>(Z);
>   } 
> 

Made it a lambda function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014