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

[Jun Zhang via Phabricator via llvm-commits]

junaire marked an inline comment as done.
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:
> 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.
Hi @bcl5980, thanks for your suggestion, I appreciate it. However, after I tried to replace `replaceInstUsesWith(*Mul, NewMul);` to `return new ICmpInst(Pred, NewMul, Mul->getOperand(1));`, the folds in my tests stop working. I took another deep look and I think your suggestion should be right, let me try to investigate why... (any more comments are welcome


================
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:
> > > 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?
OK! I updated the patch and added a test when we don't have `nsw`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210