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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb.td:1016
+
+  def : Pat<(or AddLikeOrOp:$Rn, non_imm32:$Rm), (tADDrr $Rn, $Rm)>;
+}
----------------
t.p.northover wrote:
> 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.
> you generally can't map an OR to a SUB

t2SUBri is just an ISD::ADD with a negated immediate.

> any squashable immediate would already be covered by t2ADDri

Sure, but you still save an instruction over the alternative (movw+orrs).


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb.td:1018
+// handle it a chance.
+def : T1Pat<(or AddLikeOrOp:$Rn, non_imm32:$Rm), (tADDrr $Rn, $Rm)>;
+
----------------
Maybe allow immediates not in the range 0-255 here?  Or you could just allow any operand, and depend on pattern precedence to avoid matching this when one of the immediate forms would match instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D48170