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

[Jun Zhang via Phabricator via llvm-commits]

junaire marked 8 inline comments as done.
junaire added inline comments.


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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2052
+
+  auto Reassociate = [&](Value *X, Value *Y, Constant *CI) -> llvm::Value * {
+    if (Mul->hasNoSignedWrap() && Inner->hasNoSignedWrap() &&
----------------
bcl5980 wrote:
> Do we really need llvm:: here? There is already using namespace llvm in this file at line 30.
> Do we really need llvm:: here? There is already using namespace llvm in this file at line 30.

Thanks, fixed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2071
+      if (Value *NewMul =
+              Reassociate(X, Y, ConstantInt::get(Mul->getType(), C->shl(*C))))
+        return NewMul;
----------------
goldstein.w.n wrote:
> 1 << C, not C << C?
> 1 << C, not C << C?

good catch, fixed. Thx.


================
Comment at: llvm/test/Transforms/InstCombine/icmp.ll:5345
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt <2 x i8> [[TMP1]], <i8 -1, i8 -1>
 ; CHECK-NEXT:    ret <2 x i1> [[CMP]]
 ;
----------------
goldstein.w.n wrote:
> Add some tests for the `shl` case. Better yet split adding the `shl` logic to a follow up patch.
I thought I already added tests for `mul(shl(X, C), Y)`? I don't have a strong opinion about whether we should delay `shl` support, I think that's easy to do?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210