[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);
+
----------------
junaire wrote:
> bcl5980 wrote:
> > should be 
> > ```
> > return new ICmpInst(Pred, NewMul, Mul->getOperand(1));
> > ```
> Why return it? It doesn't make sense to me. I thought what we want to do is reassociate `(X * C) * Y` to `(X * Y) * C` so the later code can recognize the pattern and fold it. Thus we reuse the existing logic.
After return you still can reuse existing. I'm not sure if currect code will break something or not but I don't think we have similar usage in instcombine.


================
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:
> > 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. 
> Yeah, we can always hoist `C` to outer mul but does that have any benefit? We hoist it here cuz we know we can eliminate it later in `foldICmpMulConstant`. Can we do the same thing for muls that doesn't have `nsw/nuw`? I took a quick look at `foldICmpMulConstant` and believe the answer it no. Correct me if I'm wrong.
That is not for foldICmpMulConstant only. Constant may match more patterns generally. For example, In foldICmpBinOp:
icmp eq/ne (X * C), (Y * C) --> icmp (X & Mask), (Y & Mask)
@nikic What do you think about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210