[PATCH] D83153: [DAGCombiner] Prevent regression in isMulAddWithConstProfitable

Thu Jul 23 16:54:55 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/urem-seteq-nonzero.ll:7-10
+; CHECK-NEXT:    mov w9, #43691
+; CHECK-NEXT:    sub w8, w0, #1 // =1
+; CHECK-NEXT:    movk w9, #43690, lsl #16
+; CHECK-NEXT:    mul w8, w8, w9
----------------
benshi001 wrote:
> benshi001 wrote:
> > efriedma wrote:
> > > spatel wrote:
> > > > I think we need to hear from someone with more AArch knowledge if this is an improvement or acceptable. cc @dmgreen @efriedma @fhahn 
> > > The new code is worse; madd is essentially the same cost as mul, so the new code has one more arithmetic instruction.  Better to spend an extra instruction materializing a constant, particularly if the code is in a loop.
> > But there are also other cases instruction count is reduced, see line 156-162 and line 176-180, I think it is optimized in total, though worse in specific cases.
> There are many aarch64 cases affected, some became better while some became worse, but they are better in total, what's your opinion?
156-162 is fewer instructions, but it's higher latency, and the current heuristic doesn't really have any way to usefully predict the result here.

I'd prefer to keep this transform off for AArch64.

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

https://reviews.llvm.org/D83153