[PATCH] D132658: [InstCombine] Distributive or+mul with const operand

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:231
+        (match(Op0, m_OneUse(m_Or(m_Value(X), m_ImmConstant(C1)))) &&
+         haveNoCommonBitsSet(X, C1, DL))) {
       Value *Mul = Builder.CreateMul(C1, Op1);
----------------
May be we need attach the context information for the function like:

```
haveNoCommonBitsSet(X, C1, DL, &AC, &I, &DT)
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:236
       if (!match(Mul, m_Mul(m_Value(), m_Value())))
         return BinaryOperator::CreateAdd(Builder.CreateMul(X, Op1), Mul);
     }
----------------
Maybe we should keep nuw flag in this change. Or add some tests and TODO for that.
https://alive2.llvm.org/ce/z/awsQrx


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:237-240
       Value *Mul = Builder.CreateMul(C1, Op1);
       // Only go forward with the transform if C1*CI simplifies to a tidier
       // constant.
       if (!match(Mul, m_Mul(m_Value(), m_Value())))
----------------
Allen wrote:
> spatel wrote:
> > The existing code is awkward. We should update it before adding to it. Use `m_ImmConstant(C1)` to do this transform but ignore a constant expression. 
> > 
> > I updated the test that was intended to go with this code change here:
> > 5260146a8a74084f3d38d8bb448ae3c5690b9084
> hi @spatel 
>    I tried with your new case, and it will fail on the check **if (!match(Mul, m_Mul(m_Value(), m_Value())))**, so the instcombine will not take active, is it within your expectations?
I think we can remove the condition:

```
 if (!match(Mul, m_Mul(m_Value(), m_Value())))
```
But I don't know what happen if (INT_MIN * -1).
Based on current folding code, Constant (INTMIN / -1) is a poison value.


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

https://reviews.llvm.org/D132658