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

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
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:
> > 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.
> 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 ? 



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

https://reviews.llvm.org/D132412