[PATCH] D25966: [AArch64] Lower multiplication by a constant int to shl+add+shl

[Renato Golin via llvm-commits]

rengolin added a comment.

Hi Haicheng,

This looks simple enough to be worth, even if the benefit is probably very small. But as it is, the code is complicating one part of two identical twins (positive and negative) and not the other, which complicates the code.

I recommend you to change that part of the code entirely into setting temporary variables, like +1/-1 ISD::ADD/ISD::SUB, based on the result of `isNonNegative`, and use the same piece of code for both paths.

A more generic approach could be done with some smarter constant-splitting, but this patch is simple as it is and there is already plenty of prior art for that, so let's stick to the pattern.

Also, I was expecting a much larger body of tests, with different constant sizes, multiple edge cases, and those that cannot be done, remaining a mul.

I have some comments inline, but my only additional question is: What is the motivation behind this? Benchmark numbers? Can you share them?

cheers,
--renato



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7552
+                  isZeroExtended(N->getOperand(0).getNode(), DAG)))
+        Lg2 = 0;
+      // Conservatively do no lower to shift+add+shift if the mul might be
----------------
why not early return? 

why is this not a problem for the previous case as well?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7561
+          }
+      APInt Shift = Value.ashr(Lg2);
       // (mul x, 2^N + 1) => (add (shl x, N), x)
----------------
`Shift` is not a good name, since this implies the "shift amount" not the "shifted value".


================
Comment at: test/CodeGen/AArch64/mul_pow2.ll:5
 ; Convert mul x, pow2 +/- 1 to shift + add/sub.
+; Convert mul x, (pow2 + 1) * pow2 to shift + add + shift.
 
----------------
You say "shift+add+shift" but your tests are on the form "add+shift".


Repository:
  rL LLVM

https://reviews.llvm.org/D25966