[PATCH] D148210: [InstCombine] Reassociate (C * X) * Y in foldICmpMulConstant

[chenglin.bi via Phabricator via llvm-commits]

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