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

[Renato Golin via llvm-commits]

rengolin added a comment.

Hi,

I have a few variable name proposals, mostly to aid the understanding of the code. I apologise for beating on that key, but the code is getting more dense, less repetitive, so it has to be well understood.

I'd welcome another review from  @Gerolf at this point. :)

cheers,
--renato



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7620
+  // TODO: consider constants in the form of (2^N - 1) * 2^M, (1-2^N) * 2^M,
+  // -(2^N+1) * 2^M, +/-(2^N +/- 1 ) * 2^M +/- 1, or (2^N +/- 1) * (2^M +/- 1).
+  auto C = dyn_cast<ConstantSDNode>(N->getOperand(1));
----------------
I think one simple formula here would be more than enough:

    (+/- 2^N +/- 1) * (+/- 2^M +/- 1)


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7628
+  SDLoc DL(N);
+  SDValue N0 = N->getOperand(0);
+  // Lg2 is used to test if the mul can be lowered to shift+add+shift.
----------------
`N0` refers to `x`, so maybe calling it `Var` or something more meaningful would be easier to understand.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7630
+  // Lg2 is used to test if the mul can be lowered to shift+add+shift.
+  unsigned Lg2 = Value.countTrailingZeros();
+  if (Lg2) {
----------------
`Lg2` implies `log base 2` of `Value`, which is not true.

`TrailingZeroes` is a better name for this.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7647
+  unsigned AddOrSub;
+  bool IsN0FirstOperand;
+  bool ExtraNeg;
----------------
Better call this `SwapValues`, as this is the intention of the flag.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7651
+    APInt VM1 = ShiftedInt - 1;
+    APInt VP1 = Value + 1;
+    if (VM1.isPowerOf2()) {
----------------
`VM1` implies `V Minus One` and `VP1` implies `V Plus One`, but the `Vs` are different.

In the case where `VM1` *is* a power of two, then `Lg2` is zero and `ShiftedInt == Value`, but not always.

I wouldn't mind `ShiftedMinus1` and `ValuePlus1`.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7657
+      AddOrSub = ISD::ADD;
+      IsN0FirstOperand = false;
+      ExtraNeg = false;
----------------
Feel free to hoist those two flags out of the conditional. This will make it clear that they're invariants here.


Repository:
  rL LLVM

https://reviews.llvm.org/D25966