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

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:10
@@ +9,3 @@
+//
+// This file defines the pass that looks through the machine instructions
+// late in the compilation, and finds byte or word instructions that
----------------
The explanation should use doxygen style comments.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:18
@@ +17,3 @@
+// of the registers.  Only instructions that have a destination register which
+// is not in any of the src registers can be affected by this.  Those
+// instructions are primarily loads, and register-to-register moves.  It would
----------------
Please explain why only those instructions can be affected.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:29
@@ +28,3 @@
+// word operations to their larger size, saving a byte in encoding. This may
+// introduce partial register dependencies, so this should only be done
+// when you can prove the partial register dependence won't exist, or when
----------------
I have to admit I don’t remember what the documentation says, but when we write a 16-bit register we may already have partial register dependencies, right?

What I want to point out is that this comment seems to go against one of the initial statement that this pass will avoid false-dependencies on the upper portions of the registers. I think this deserves more explanation like when we do kill those false-dependencies and when we might introduce new ones. No need to be fancy here, I believe something simply stating that some instructions set the upper bits and some don’t and thus have false-dependencies would be sufficient. 

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:73
@@ +72,3 @@
+
+  /// \brief This sets the SuperDestReg to the 32 bit super reg
+  /// of the original destination register of the MachineInstr
----------------
\p SuerDestReg

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:77
@@ +76,3 @@
+  /// just prior to OrigMI, and false if not.
+  bool getSuperRegDestIfDead(MachineInstr *OrigMI, unsigned OrigDestSize,
+                             unsigned &SuperDestReg) const;
----------------
I am assuming we want something like that as well
/// \pre OrigDestSize is a 8 or 16-bit.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:90
@@ +89,3 @@
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<MachineLoopInfo>(); //  Need machine loop info.
+    MachineFunctionPass::getAnalysisUsage(AU);
----------------
Either don’t put this comment or explain why we need it.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:101
@@ +100,3 @@
+  MachineFunction *MF;
+  const X86InstrInfo *TII; // Machine instruction info.
+  bool OptForSize;
----------------
We usually write the comment with doxygen style and before the field. 

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:132
@@ +131,3 @@
+/// This function returns true if the super-register of the destination byte
+/// or word register is dead immediately before the machine instruction.
+///
----------------
Don’t put doxygen comments on both the declaration and definition.
I think doxygen does not like it and also it means we have two spots to update when we need to change the documentation.

================
Comment at: lib/Target/X86/X86FixupBWInsts.cpp:197
@@ +196,3 @@
+
+  MIB->setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
+
----------------
We need to copy the implicit operands as well.
Look for:
MachineInstr::copyImplicitOps


http://reviews.llvm.org/D17032