[PATCH] D148210: [InstCombine] Reassociate (C * X) * Y in foldICmpMulConstant
chenglin.bi via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 20 01:18:14 PDT 2023
bcl5980 added inline comments.
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2105
+ if (Value *NewMul = tryReassociateMulConstant(Pred, Mul, Builder))
+ replaceInstUsesWith(*Mul, NewMul);
+
----------------
should be
```
return new ICmpInst(Pred, NewMul, Mul->getOperand(1));
```
================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2043
+// Reassociate (C * X) * Y to (X * Y) * C to enable further
+// optimizations.
----------------
junaire wrote:
> bcl5980 wrote:
> > I'm not sure what are you going to do now. If you just want to reassociate C to outer mul, there is no extra condition. https://alive2.llvm.org/ce/z/v-f92Q
> > The code should be simple:
> >
> > ```
> > BinaryOperator *InnerMul;
> > Value *Y;
> > Constant *InnerC;
> > if (match(Mul, m_OneUse(m_c_Mul(m_BinOp(InnerMul), m_Value(Y)))) &&
> > match(InnerMul, m_Mul(m_Value(X), m_Constant(InnerC)))) {
> > Value *NewInner = Builder.CreateMul(X, Y);
> > Value *NewMul = Builder.CreateMul(NewInner, InnerC);
> > return new ICmpInst(Pred, NewMul, Mul->getOperand(1));
> > }```
> > If you want to keep nsw/nuw flags, you can set the flag after create NewMul like:
> >
> > ```
> > if (!InnerC->isZeroValue() && Mul->hasNoUnsignedWrap() &&
> > InnerMul->hasNoUnsignedWrap()) {
> > cast<BinaryOperator>(NewInner)->setHasNoUnsignedWrap();
> > cast<BinaryOperator>(NewMul)->setHasNoUnsignedWrap();
> > } else {
> > Constant *Zero = Constant::getNullValue(InnerC->getType());
> > Constant *InnerCSleZero =
> > ConstantExpr::getCompare(ICmpInst::ICMP_SLE, InnerC, Zero);
> > if (InnerCSleZero->isZeroValue() && Mul->hasNoSignedWrap() &&
> > InnerMul->hasNoSignedWrap()) {
> > cast<BinaryOperator>(NewInner)->setHasNoSignedWrap();
> > cast<BinaryOperator>(NewMul)->setHasNoSignedWrap();
> > }
> > }
> > ```
> >
> IIUC we want to reassociate `(X * C) * Y` to `(X * Y) * C` if they both has nsw/nuw flags so they can be folded by the existing logic in `foldICmpMulConstant`.
> > I'm not sure what are you going to do now. If you just want to reassociate C to outer mul, there is no extra condition. https://alive2.llvm.org/ce/z/v-f92Q
> > The code should be simple:
> >
> > ```
> > BinaryOperator *InnerMul;
> > Value *Y;
> > Constant *InnerC;
> > if (match(Mul, m_OneUse(m_c_Mul(m_BinOp(InnerMul), m_Value(Y)))) &&
> > match(InnerMul, m_Mul(m_Value(X), m_Constant(InnerC)))) {
> > Value *NewInner = Builder.CreateMul(X, Y);
> > Value *NewMul = Builder.CreateMul(NewInner, InnerC);
> > return new ICmpInst(Pred, NewMul, Mul->getOperand(1));
> > }```
>
> The issue is that if the inner mul's constant operand is a power of 2, it will be folded to an `shl`. like:
> ```
> %mul = shl nsw i8 %a, i8 1
> %mul2 = mul nsw i8 %b, %mul
> ```
> Then the pattern match you proposed doesn't work anymore.
> > If you want to keep nsw/nuw flags, you can set the flag after create NewMul like:
> >
> > ```
> > if (!InnerC->isZeroValue() && Mul->hasNoUnsignedWrap() &&
> > InnerMul->hasNoUnsignedWrap()) {
> > cast<BinaryOperator>(NewInner)->setHasNoUnsignedWrap();
> > cast<BinaryOperator>(NewMul)->setHasNoUnsignedWrap();
> > } else {
> > Constant *Zero = Constant::getNullValue(InnerC->getType());
> > Constant *InnerCSleZero =
> > ConstantExpr::getCompare(ICmpInst::ICMP_SLE, InnerC, Zero);
> > if (InnerCSleZero->isZeroValue() && Mul->hasNoSignedWrap() &&
> > InnerMul->hasNoSignedWrap()) {
> > cast<BinaryOperator>(NewInner)->setHasNoSignedWrap();
> > cast<BinaryOperator>(NewMul)->setHasNoSignedWrap();
> > }
> > }
> > ```
> >
SHL is not the main thing I discuss here, we can support shl similar to mul. What I mean is do we really need to limit the transform to nsw/nuw. Always hoist C to outer mul should be a more general solution I think.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148210/new/
https://reviews.llvm.org/D148210
More information about the llvm-commits
mailing list