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

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

The code LGTM, modulo minor comments.
Regarding the code sequence itself, I still suggest you wait for input from Dave.


================
Comment at: lib/Target/X86/X86InstrCompiler.td:271
@@ +270,3 @@
+                       [(set GR64:$dst, i64immSExt8:$src)]>, Requires<[OptForSize]>;
+// XXX: Is leaving out the instruction itinerary class and Schedule OK?
+}
----------------
It would probably be a good idea to add a schedule, but I'm not sure what it ought to be.
I'm fine with leaving it as a TODO.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:5260
@@ +5259,3 @@
+  MachineFunction &MF = *MBB.getParent();
+  if (!TFL->hasFP(MF) && MF.getMMI().usePreciseUnwindInfo()) {
+    TFL->BuildCFI(MBB, I, DL,
----------------
This isn't quite right - usePreciseUnwindInfo() is meant to indicate that we should be using precise CFI *if* we use CFI. It doesn't say anything about whether we should be using CFI at all. The way to determine whether CFI is needed is something like this (taken from FrameLowering):

bool IsWin64Prologue = MF.getTarget().getMCAsmInfo()->usesWindowsCFI();
bool NeedsDwarfCFI =
      !IsWin64Prologue && (MMI.hasDebugInfo() || Fn->needsUnwindTableEntry());

What you want here is "!TFL->hasFP(MF) && NeedsDwarfCFI && MF.getMMI().usePreciseUnwindInfo()"
This probably ought to be better encapsulated somewhere, but that's a different patch.

================
Comment at: test/CodeGen/X86/movtopush.ll:117-118
@@ -116,3 +116,4 @@
 ; NORMAL-LABEL: test4:
-; NORMAL: movl    $2, %eax
+; NORMAL: pushl $2
+; NORMAL: popl %eax
 ; NORMAL-NEXT: pushl   $4
----------------
Not sure. CDQ creates a dependency between the two register writes. as opposed to two independent moves.
But using two push/pop pairs probably does as well. And CDQ *is* smaller...


http://reviews.llvm.org/D14971