[PATCH] D17032: [X86] Add a pass to change byte and word instructions to zero-extending versions.

[Eric Christopher via llvm-commits]

echristo accepted this revision.
echristo added a reviewer: echristo.
echristo added a comment.
This revision is now accepted and ready to land.

Seems reasonable to me. Note that I haven't looked at it much more than the few inline comments I just gave, but it seems good to me - especially as it isn't turned on by default.


================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:137-138
@@ +136,4 @@
+/// reference for the instruction, and still be killed/last used by the
+/// instruction. However, the existing query interfaces don't seem to
+/// easily allow that to be checked.
+///
----------------
Can you elaborate on this as far as what kind of interface you'd need?

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:184
@@ +183,3 @@
+  unsigned NewDestReg;
+
+  if (!getSuperRegDestIfDead(MI, OrigDestSize, NewDestReg))
----------------
Comment here as to why this is a sufficient check please? I've seen the comment above, but...

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:189
@@ +188,3 @@
+  // Safe to change the instruction.
+
+  MachineInstrBuilder MIB =
----------------
Extra whitespace.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:205
@@ +204,3 @@
+
+  //
+  // This algorithm doesn't delete the instructions it is replacing
----------------
Delete.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:216
@@ +215,3 @@
+  // from making it seem as if the larger register might be live.
+  //
+  SmallVector<std::pair<MachineInstr *, MachineInstr *>, 8> MIReplacements;
----------------
Delete.


http://reviews.llvm.org/D17032