[PATCH] D19999: [X86] Teach X86FixupBWInsts to promote MOV8rr/MOV16rr to MOV32rr.

[Kevin B. Smith via llvm-commits]

kbsmith1 added a comment.

The code generally looks good.  I don't see any tests that explicitly test this code though:

if (TRI->getSubRegIndex(NewSrcReg, OldSrc.getReg()) !=

      TRI->getSubRegIndex(NewDestReg, OldDest.getReg()))
  return nullptr;

Definitely need to be sure that this works, and has tests.  Don't want

movb %ah, %cl

to accidentally be turned into

movl %eax, %ecx


================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:98
@@ -97,1 +97,3 @@
 
+  /// \brief Change the MachineInstr \p MI into the equivalent 32-bit copy
+  /// if it is safe to do so.  Return the replacement instruction if OK,
----------------
I think \brief is no longer needed.  I realize the others have it.  I was just recently told of this, but haven't removed the others.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:249
@@ +248,3 @@
+      BuildMI(*MF, MI->getDebugLoc(), TII->get(X86::MOV32rr), NewDestReg)
+          .addOperand(OldSrc);
+  MIB->getOperand(1).setReg(NewSrcReg);
----------------
I think this would be better as .addReg(NewSrcReg); and then the next line can be deleted.


http://reviews.llvm.org/D19999