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

[Jun Zhang via Phabricator via llvm-commits]

junaire added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2105
+  if (Value *NewMul = tryReassociateMulConstant(Pred, Mul, Builder))
+    replaceInstUsesWith(*Mul, NewMul);
+
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2043
 
+// Reassociate (C * X) * Y to (X * Y) * C to enable further
+// optimizations.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210