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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n 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)))) ||
----------------
sdesmalen wrote:
> 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;
> It might be a bit easier to follow, if you explicitly do the scaling while doing the matching, i.e.

Inlined the ShiftY/ShiftZ

Prefer keeping the 4 explicit cases (removed the ShiftX case and moved to next patch). Think its clearer to have the cases each explicitly laid out, rather than having nest if/else statements. LMK if thats okay, will change if you feel strongly.
> 
>   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;
----------------
sdesmalen wrote:
> 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`. 
> 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`. 

There are some tests for it, though they are all non-constant cases (so it starts to matter in the next patch).

I split it to a new patch (see test=D147107, impl=D147108) and I added tests for the constant version of case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144225