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

Thu Jul 23 18:16:18 PDT 2020

benshi001 abandoned this revision.
benshi001 marked an inline comment as done.
benshi001 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
----------------
efriedma wrote:
> 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.
I see. Thanks.

I also have other patches for ARM. And you are appreciated to help me review them.

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

https://reviews.llvm.org/D83153