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

[Allen zhong via Phabricator via llvm-commits]

Allen marked 3 inline comments as done.
Allen 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);
----------------
bcl5980 wrote:
> May be we need attach the context information for the function like:
> 
> ```
> haveNoCommonBitsSet(X, C1, DL, &AC, &I, &DT)
> ```
Apply your comment, thanks


================
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);
     }
----------------
bcl5980 wrote:
> Maybe we should keep nuw flag in this change. Or add some tests and TODO for that.
> https://alive2.llvm.org/ce/z/awsQrx
I make a try to propagation the flag, thanks


================
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())))
----------------
bcl5980 wrote:
> 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.
the above checking is deleted, and now **i32 ptrtoint (i32* @g to i32)** doesn't match the  **match(Op1, m_ImmConstant())** for that case, which I think it is within expectations ?


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

https://reviews.llvm.org/D132658