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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1725-1738
+  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();
----------------
goldstein.w.n wrote:
> MattDevereau wrote:
> > We can do
> > 
> > ```
> >   ConstantInt *ConstY, *ConstZ;
> >   Constant *CY, *CZ;
> >   if (!I.getType()->isVectorTy() || !match(Y, m_Constant(CY)) ||
> >       !match(CY->getSplatValue(), m_ConstantInt(ConstY)) ||
> >       !match(Z, m_Constant(CZ)) ||
> >       !match(CZ->getSplatValue(), m_ConstantInt(ConstZ)))
> >     if (!match(Y, m_ConstantInt(ConstY)) || !match(Z, m_ConstantInt(ConstZ)))
> >       return nullptr;
> > ```
> > 
> > Which saves us a few lines, and this also lets us remove the later check for
> > 
> > ```
> >   if (!ConstY || !ConstZ)
> >     return nullptr;
> > ```
> Prefer not because the generic patch doesn't want to bail out if we can't find constants.
> We can do
> 
> ```
>   ConstantInt *ConstY, *ConstZ;
>   Constant *CY, *CZ;
>   if (!I.getType()->isVectorTy() || !match(Y, m_Constant(CY)) ||
>       !match(CY->getSplatValue(), m_ConstantInt(ConstY)) ||
>       !match(Z, m_Constant(CZ)) ||
>       !match(CZ->getSplatValue(), m_ConstantInt(ConstZ)))
>     if (!match(Y, m_ConstantInt(ConstY)) || !match(Z, m_ConstantInt(ConstZ)))
>       return nullptr;
> ```
>

Irreverent of keeping aligned with the generic patch, I find that logic much harder to follow. There are alot of conditions and non-intuitive short-circuit requirements. 
> Which saves us a few lines, and this also lets us remove the later check for
> 
> ```
>   if (!ConstY || !ConstZ)
>     return nullptr;
> ```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014