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

[Chad Rosier via llvm-commits]

mcrosier accepted this revision.
mcrosier added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits about naming of variables.



================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7633
+  // which equals to (1+2)*16-(1+2).
+  SDValue Var = N->getOperand(0);
+  // TrailingZeroes is used to test if the mul can be lowered to
----------------
'Var' isn't descriptive.  I'd prefer 'N0' as this is a common idiom in ISel lowering.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7649
+  }
+  // Use ConstantA instead of ConstValue to support both shift+add/sub and
+  // shift+add+shift.
----------------
I'd prefer 'ShiftedConstValue' over 'ConstantA'.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7663
+    // (mul x, (2^N + 1) * 2^M) => (shl (add (shl x, N), x), M)
+    APInt CAMinus1 = ConstantA - 1;
     APInt CVPlus1 = ConstValue + 1;
----------------
CAMinus1 -> SCVMinus1


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7665
     APInt CVPlus1 = ConstValue + 1;
-    if (CVMinus1.isPowerOf2()) {
-      ShiftAmt = CVMinus1.logBase2();
+    if (CAMinus1.isPowerOf2()) {
+      ShiftAmt = CAMinus1.logBase2();
----------------
CAMinus1 -> SCVMinus1


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7692
   EVT VT = N->getValueType(0);
-  SDValue N0 = N->getOperand(0);
-  SDValue ShiftedVal = DAG.getNode(ISD::SHL, DL, VT, N->getOperand(0),
+  SDValue ShiftedVal = DAG.getNode(ISD::SHL, DL, VT, Var,
                                    DAG.getConstant(ShiftAmt, DL, MVT::i64));
----------------
Var -> N0


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7695
 
-  SDValue AddSubN0 = ShiftValUseIsN0 ? ShiftedVal : N0;
-  SDValue AddSubN1 = ShiftValUseIsN0 ? N0 : ShiftedVal;
+  SDValue AddSubN0 = ShiftValUseIsN0 ? ShiftedVal : Var;
+  SDValue AddSubN1 = ShiftValUseIsN0 ? Var : ShiftedVal;
----------------
Var -> N0


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7696
+  SDValue AddSubN0 = ShiftValUseIsN0 ? ShiftedVal : Var;
+  SDValue AddSubN1 = ShiftValUseIsN0 ? Var : ShiftedVal;
   SDValue Res = DAG.getNode(AddSubOpc, DL, VT, AddSubN0, AddSubN1);
----------------
Var -> N0


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7698
   SDValue Res = DAG.getNode(AddSubOpc, DL, VT, AddSubN0, AddSubN1);
-  if (!NegateResult)
+  if (TrailingZeroes == 0 && !NegateResult)
     return Res;
----------------
Please add an assert showing that TrailingZeroes and NegateResult can't both be true.

Then the last bit of logic here can be written as:

  // Negate the result.
  if (NegateResult)
    return DAG.getNode(ISD::SUB, DL, VT, DAG.getConstant(0, DL, VT), Res);

  // Shift the result.
  if (TrailingZeroes)
    return DAG.getNode(ISD::SHL, DL, VT, Res, DAG.getConstant(TrailingZeroes, DL, MVT::i64));

  return Res;


Repository:
  rL LLVM

https://reviews.llvm.org/D25966