[PATCH] D132412: [InstCombine] reassociate/factor add+mul with common operand

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1423
+    Value *IncY = Builder.CreateAdd(Y, ConstantInt::get(I.getType(), 1));
+    return BinaryOperator::CreateMul(IncY, X);
+  }
----------------
Allen wrote:
> should we need consider the flag of NUW and NSW ?
I only found one case so far where we can partially propagate nuw:
https://alive2.llvm.org/ce/z/aGSyrK
Everything else seems to be illegal. Propagating the flag would be another reason to code this match explicitly rather than trying to make it work through tryFactorization().


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1422
+                        m_OneUse(m_c_Add(m_Deferred(X), m_Value(Z)))))) {
+    // (X * Y) + (X + Z) --> ((Y + 1) * X) + Z
+    Value *IncY = Builder.CreateAdd(Y, ConstantInt::get(I.getType(), 1));
----------------
bcl5980 wrote:
> spatel wrote:
> > Allen wrote:
> > > bcl5980 wrote:
> > > > hiraditya wrote:
> > > > > Can this cause a regression when Y  is a power of two?
> > > > > Some other optimization would rewrite `x*y` as `y << n (when n == lg(y) at compile time)`?
> > > > https://alive2.llvm.org/ce/z/LRobTp
> > > > It looks the answer is yes.
> > > so, the decomposition the const of a MUL should be improved first?
> > That is correct - this transform could obscure a power-of-2 transform. 
> > But it could also expose a power-of-2 multiply that we miss today:
> > https://alive2.llvm.org/ce/z/ZUjR5S
> > 
> > In the typical instcombine sequence, we would process the multiply operand before the add user of that operand, so I haven't found a way to show a regression from this patch yet. 
> > 
> > We need a more general solution if we are going to get the optimal IR for all patterns.
> > 
> > But the example shows a simpler pattern that I missed initially:
> > https://alive2.llvm.org/ce/z/eMhvQa
> > 
> > I think we should do that transform for the same reason stated here - it reduces the number of uses of a variable operand.
> > 
> > And it looks like we might not need this patch because `-reassociate` gives us that simpler pattern.
> This looks like a special case of distributive law: x * y + x * 1
> How about move this to InstCombinerImpl::SimplifyUsingDistributiveLaws or InstCombinerImpl::tryFactorization ? 
> 
We do try to match this pattern in that code, but we miss it because the operand 'x' has multiple uses. I'll try to make it work if that isn't too ugly...but this explicit pattern-match might be the clearer way to achieve the transform.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1422
+                        m_OneUse(m_c_Add(m_Deferred(X), m_Value(Z)))))) {
+    // (X * Y) + (X + Z) --> ((Y + 1) * X) + Z
+    Value *IncY = Builder.CreateAdd(Y, ConstantInt::get(I.getType(), 1));
----------------
spatel wrote:
> bcl5980 wrote:
> > spatel wrote:
> > > Allen wrote:
> > > > bcl5980 wrote:
> > > > > hiraditya wrote:
> > > > > > Can this cause a regression when Y  is a power of two?
> > > > > > Some other optimization would rewrite `x*y` as `y << n (when n == lg(y) at compile time)`?
> > > > > https://alive2.llvm.org/ce/z/LRobTp
> > > > > It looks the answer is yes.
> > > > so, the decomposition the const of a MUL should be improved first?
> > > That is correct - this transform could obscure a power-of-2 transform. 
> > > But it could also expose a power-of-2 multiply that we miss today:
> > > https://alive2.llvm.org/ce/z/ZUjR5S
> > > 
> > > In the typical instcombine sequence, we would process the multiply operand before the add user of that operand, so I haven't found a way to show a regression from this patch yet. 
> > > 
> > > We need a more general solution if we are going to get the optimal IR for all patterns.
> > > 
> > > But the example shows a simpler pattern that I missed initially:
> > > https://alive2.llvm.org/ce/z/eMhvQa
> > > 
> > > I think we should do that transform for the same reason stated here - it reduces the number of uses of a variable operand.
> > > 
> > > And it looks like we might not need this patch because `-reassociate` gives us that simpler pattern.
> > This looks like a special case of distributive law: x * y + x * 1
> > How about move this to InstCombinerImpl::SimplifyUsingDistributiveLaws or InstCombinerImpl::tryFactorization ? 
> > 
> We do try to match this pattern in that code, but we miss it because the operand 'x' has multiple uses. I'll try to make it work if that isn't too ugly...but this explicit pattern-match might be the clearer way to achieve the transform.
Ideally yes, we would have decomposition completed in the backend. But I also think this patch can uncover folds that we miss today (like the example where we created a power-of-2 multiply). So this patch should not be blocked on improving the backend. There could be both wins and losses.


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

https://reviews.llvm.org/D132412