[PATCH] D48170: ARM: use "add" instead of "orr" for code size

Tim Northover via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 18 03:16:29 PDT 2018


t.p.northover added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb.td:1016
+
+  def : Pat<(or AddLikeOrOp:$Rn, non_imm32:$Rm), (tADDrr $Rn, $Rm)>;
+}
----------------
efriedma wrote:
> t.p.northover wrote:
> > efriedma wrote:
> > > T1Pat?
> > > 
> > > Are you intentionally avoiding transforming `x|256`?
> > > T1Pat?
> > 
> > That seems to require "IsThumb1Only", which I don't want.
> > 
> > > Are you intentionally avoiding transforming `x|256`?
> > 
> > Yes. That would result in a `MOV`/`ADD` pair (with restricted register classes). With this patch we get `ORR.W` with an immediate, which is probably better all round.
> > 
> > It's a bit of a heuristic, and not optimal when both `ADD` and `ORR` require a separate immediate. But by that point the only benefit is whether the destination register is tied so I decided to draw the line before that point in my initial patch.
> > That seems to require "IsThumb1Only", which I don't want.
> 
> I think it's appropriate.
> 
> The way ISel for Thumb2 works is that we never select Thumb1 instructions, which have restrictive register allocation requirements (most instructions only take tGPR).  Thumb2SizeReduce runs after register allocation and shrinks the instructions which are equivalent to a Thumb instruction.  (This sometimes leads to bad register allocation decisions because the register allocator isn't aware of size reduction, but that's orthogonal to this patch.)
> 
> So in Thumb2 mode, you want five patterns, selecting the following five instructions: t2ADDri, t2ADDri12, t2SUBri, t2SUBri12, and t2ADDrr. And you don't want any patterns to select tADDi3, tADDi8, tSUBi8, or tADDrr.
OK, I'm convinced by your arguments on the Thumb1 side, I hadn't quite realised that was how it worked. But I don't think t2 side is quite right.

For a start, you generally can't map an OR to a SUB (maybe an XOR, but that's getting pretty convoluted). Also, I don't think the t2ADDri12 is needed: any squashable immediate would already be covered by t2ADDri.

t2ADDrr is probably what I had forgotten and led me to my Thumb1 mistake. I'll upload a new patch implementing those bits.


Repository:
  rL LLVM

https://reviews.llvm.org/D48170





More information about the llvm-commits mailing list