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

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.

I thought I understand this until about the middle of the review.  Now I could use some help perhaps with variable names and comments that reflect more clearly on the expression(s) you simplify. I think this is what Renato is looking for, too.

Thanks
Gerolf



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7617
   // 64-bit is 5 cycles, so this is always a win.
-  if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(N->getOperand(1))) {
-    const APInt &Value = C->getAPIntValue();
-    EVT VT = N->getValueType(0);
-    SDLoc DL(N);
-    if (Value.isNonNegative()) {
+  // More aggressively, some multiplications can be lowered to shift+add+shift
+  // if the constant is (2^N + 1) * 2^M.
----------------
You could tie it more to the code, e.g. some multiplications Var * C can be ...




================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7620
+  // TODO: consider lowering more cases, e.g. constants in the form of
+  // (+/-2^N+/-1)*(+/-2^M+/-1).
+  auto C = dyn_cast<ConstantSDNode>(N->getOperand(1));
----------------
I liked the spaces in Renato's comment. Or would it be clearer to say " constants C = A*B where A, B are of type +/- (2^N +/- 1)"?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7625
+
+  const APInt &Value = C->getAPIntValue();
+  EVT VT = N->getValueType(0);
----------------
ValueOfC?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7633
+  if (TrailingZeroes) {
+    // Conservatively do no lower to shift+add+shift if the mul might be
+    // folded into smul or umul.
----------------
no -> not


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7638
+      return SDValue();
+    // Conservatively do no lower to shift+add+shift if the mul might be
+    // folded into madd or msub.
----------------
dito



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7644
+  }
+  APInt ShiftedInt = Value.ashr(TrailingZeroes);
+
----------------
If you declare e.g C = A * B then ShiftedInt could be ConstantA etc


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7647
+  APInt IntValue;
+  unsigned AddOrSub;
+  bool SwapValues;
----------------
Operation would be more general than AddOrSub


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7649
+  bool SwapValues;
+  bool ExtraNeg;
+  if (Value.isNonNegative()) {
----------------
Please add a comment like what values get swapped.
ExtraNeg -> NegExp? Or NegSubExp?


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7651
+  if (Value.isNonNegative()) {
+    APInt ShiftedMinus1 = ShiftedInt - 1;
+    APInt ValuePlus1 = Value + 1;
----------------
ShiftedMinus1 could be ConstandAMinus1


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7652
+    APInt ShiftedMinus1 = ShiftedInt - 1;
+    APInt ValuePlus1 = Value + 1;
+    SwapValues = false;
----------------
Shouldn't ValuePlus1 be ConstantAPlus1? But then it should be ConstantAPlus1 = ShiftedInt (ConstantA) -1.  At this point I can't match your specification and your code. However, if I"m right about this I will need to dig deep into your test cases, too ...


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7665
+    } else
+      return SDValue();
+  } else {
----------------
After this point I think you can assert(IntValue == 2^N, some power of 2). 


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7667
+  } else {
+    APInt NegativeValuePlus1 = -Value + 1;
+    APInt NegativeValueMinus1 = -Value - 1;
----------------
I think Value should be ShiftedMinus1 from here on.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7684
   }
-  return SDValue();
+  SDValue ShiftedVal =
+      DAG.getNode(ISD::SHL, DL, VT, Var,
----------------
I'll take another look at this code after I (think I) understand the code above.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7690
+                  SwapValues ? ShiftedVal : Var);
+  if (TrailingZeroes)
+    return DAG.getNode(ISD::SHL, DL, VT, AddOrSubVal,
----------------
It is not clear to me why TrailingZeros and ExtraNeg are exclusive.


Repository:
  rL LLVM

https://reviews.llvm.org/D25966