[PATCH] D14971: X86: Emit smaller code for moving 8-bit immediates

[David Kreitzer via llvm-commits]

DavidKreitzer added inline comments.

================
Comment at: lib/Target/X86/X86InstrCompiler.td:270
@@ +269,3 @@
+  let Defs = [EFLAGS], isReMaterializable = 1, isPseudo = 1 in {
+    def MOV32r1 : I<0, Pseudo, (outs GR32:$dst), (ins), "",
+                        [(set GR32:$dst, 1)]>;
----------------
hans wrote:
> DavidKreitzer wrote:
> > I certainly can't argue with the logic that what you are adding is consistent with MOV32r0. And from that perspective, I think your patch is fine the way it is. I just wonder if using MOV32ri universally and having a late expansion that chooses between the many possible implementations is a better long term direction.
> > 
> > I understand your point about the EFLAGS clobber, but note that the pessimism works both ways.  The MOV32r0 prevents transformations that would make EFLAGS live across it, e.g. to remove a redundant compare. We'd probably prefer to remove the redundant compare & use "mov $0, %eax" to generate the 0 than leave the redundant compare & use "xor %eax, %eax". (Of course, you could specifically check for MOV32r0 and convert it to MOV32ri when the transformation happens just like X86InstrInfo::rematerialize is doing, but you could do the same sort of thing with the MOV32ri representation.)
> I don't really like the idea of having a pseudo instruction expand in (too many) different ways, but I also feel I don't have enough experience in the backend to argue my case very well :-)
> 
> Already, I would have preferred to just do this with patterns, but I think we need the pseudo instruction for rematerialization to work.
> 
> But to have a generic MOV32ri that would expand to different things seems like it's almost working against the code generator somehow. For example, how can it get scheduled effectively?
> 
> Also, checking whether EFLAGS is live wouldn't be very efficient. For example, X86InstrInfo::isSafeToClobberEFLAGS only considers a few instructions in each direction for efficiency. I would much prefer to have EFLAGS taken into account when choosing to use the instruction.
> 
> 
> I still prefer the current patch. We could later add another pseudo for the push/pop lowering with minsize, and then try to figure out the best way to handle 64-bit.
That works for me. Your concern about EFLAGS is definitely justified. And this is small enough in scope that we can easily make changes, if necessary.



http://reviews.llvm.org/D14971