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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think it would be better to make the 'nuw' change separately. That is missing from the current transform, so it could be done before this patch with 'add' alone, or make that a follow-up patch to this one.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:232
+         haveNoCommonBitsSet(X, C1, DL, &AC, &I, &DT))) {
       Value *Mul = Builder.CreateMul(C1, Op1);
+      // C1*CI simplifies to a tidier constant.
----------------
It would be clearer to capture Op1 as a constant and create this constant directly:
  Constant *MulC;
  if (match(Op1, m_ImmConstant(MulC))) {
    ...
    Constant *NewC = ConstantExpr::getMul(C1, MulC);

Please adjust the code comment above here to match whatever variable names you choose. So it may look like this:
  // (X + C1) * MulC --> (X * MulC) + (C1 * MulC)


================
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:
> 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 ?
Yes, that is the expectation - the transform should not be attempted with constant expressions.


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

https://reviews.llvm.org/D132658