[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:1721
+  // If X is constant 1, then we avoid both in the mul and shl case.
+  auto *CX = dyn_cast<Constant>(X);
+  if (CX && CX->isOneValue())
----------------
MattDevereau wrote:
> The `*` here is not needed 
I prefer to keep `*` when its a pointer, think its clearer.


================
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();
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1742
+  }
+  bool IsSigned = I.getOpcode() == Instruction::SRem;
+
----------------
MattDevereau wrote:
> MattDevereau wrote:
> > I think `IsSRem` is more descriptive and clearer to read later on. Now that we've removed the nested if statements we can move this assignment directly before its first use.
> This comment isn't done, IsSRem is still defined far before it is first used
Prefer not to change because the generic patch needs it declared before the `if(ConstY && ConstZ) {...}`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1743-1747
+
+  bool NSW0 = BO0->hasNoSignedWrap();
+  bool NSW1 = BO1->hasNoSignedWrap();
+  bool NUW0 = BO0->hasNoUnsignedWrap();
+  bool NUW1 = BO1->hasNoUnsignedWrap();
----------------
MattDevereau wrote:
> MattDevereau wrote:
> > For these names maybe call them something like `BO0HasNSW`, `BO1HasNSW` to make it read a bit more literally and lessen the reader's memory overhead of what the 0 in NSW0 represents. We can move these assignments directly before their first use now.
> This comment isn't done, these variables are still defined far before they are used.
Prefer not to change because the generic patch needs it declared before the `if(ConstY && ConstZ) {...}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143014