[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:2043
 
+// Reassociate (C * X) * Y to (X * Y) * C to enable further
+// optimizations.
----------------
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();
      }
    }
```



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210