[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: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:
> 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.


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

https://reviews.llvm.org/D132412