[PATCH] D143014: [InstCombine] Add combines for `(urem/srem (mul/shl X, Y), (mul/shl X, Z))`
Noah Goldstein via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 9 08:57:36 PST 2023
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
More information about the llvm-commits
mailing list