[PATCH] D132322: [AArch64][SelectionDAG] Optimize multiplication by constant

[Allen zhong via Phabricator via llvm-commits]

Allen marked 4 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14562
+      // the operand of madd/msub should not be const value.
+      if (!AddSub->hasOneUse() || !isIntOrFPConstant(AddSub->getOperand(1)))
+        return SDValue();
----------------
efriedma wrote:
> dmgreen wrote:
> > I don't think it matters it the AddSub has one use? (It might for some other optimizations, but that can be a separate patch).
> > 
> > It matters that for a sub the `AddSub->operand(1) == N`.  If it is `AddSub->operand(0) == N`, then we cannot fold to a msub, so that shouldn't block the transform to add+shifts.
> > The isIntOrFPConstant check can check for isLegalAddImmediate, to be a little more precise where it will generate a mul+addri or subri.
> Do you want to check specifically for a constant that fits into the immediate operand of an add/sub?  If it doesn't fit, it doesn't really matter if it's constant.
Thanks @dmgreen and @efriedma for detail suggestion


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14617
+                                    DAG.getConstant(ShiftAmt, DL, MVT::i64));
+  SDValue ShiftedVal1 = DAG.getNode(
+      ISD::SHL, DL, VT, N0, DAG.getConstant(TrailingZeroes, DL, MVT::i64));
----------------
efriedma wrote:
> Move computation of "ShiftedVal1" into the if statment?
Done ,thanks


================
Comment at: llvm/test/CodeGen/AArch64/mul_pow2.ll:146-148
+; CHECK-NEXT:    lsl w8, w0, #2
+; CHECK-NEXT:    add w8, w8, w0, lsl #1
+; CHECK-NEXT:    add w0, w8, w1
----------------
dmgreen wrote:
> I think that (considering the mov as free in terms of latency), in this case the madd would be worth 2-3, the lsl+add_lsl+add would cost 3-4. It would depend heavily on the exact cpu though.
> For i64 muls the madd would have a higher cost (it was 4 on one cpu I tested, but newer cpus are better).
done, block the change as we can make use of madd


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

https://reviews.llvm.org/D132322