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

[Allen zhong via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13712
+    return false;
+  SDNode *Mul = *C->use_begin();
+  // Block the SExt/ZExt as ADD/SUB with sxtw/zxtw has low throughput.
----------------
dmgreen wrote:
> We can't just expect the first user of a const will be the mul we are interested in.
> 
> All the other handling of decomposing mul into add+shift is currently done in performMulCombine. Would it be better to just alter the code that is already there? It would make it easier to be more precise with the costmodel.
Apply your commend, and refactor it with costmodel, thanks


================
Comment at: llvm/test/CodeGen/AArch64/mul_pow2.ll:309
+; GISEL-NEXT:    ret
+  %mul = mul nsw i32 %x, 6
+  %sub = add nsw i32 %mul, -1
----------------
dmgreen wrote:
> Is this the case you are interested in? Could we change the existing costmodel to be more precise with which sub operand it considers free?
> ```
>     // Conservatively do not lower to shift+add+shift if the mul might be
>     // folded into madd or msub.
>     if (N->hasOneUse() && (N->use_begin()->getOpcode() == ISD::ADD ||
>                            N->use_begin()->getOpcode() == ISD::SUB))
>       return SDValue();
> ```
Thanks for your idea, I can try this, but why the sub operand can be considered free ?


================
Comment at: llvm/test/CodeGen/AArch64/mul_pow2.ll:309
+; GISEL-NEXT:    ret
+  %mul = mul nsw i32 %x, 6
+  %sub = add nsw i32 %mul, -1
----------------
Allen wrote:
> dmgreen wrote:
> > Is this the case you are interested in? Could we change the existing costmodel to be more precise with which sub operand it considers free?
> > ```
> >     // Conservatively do not lower to shift+add+shift if the mul might be
> >     // folded into madd or msub.
> >     if (N->hasOneUse() && (N->use_begin()->getOpcode() == ISD::ADD ||
> >                            N->use_begin()->getOpcode() == ISD::SUB))
> >       return SDValue();
> > ```
> Thanks for your idea, I can try this, but why the sub operand can be considered free ?
Done, thanks


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

https://reviews.llvm.org/D132322