[PATCH] D21726: CodeGen: Use MachineInstr& in TargetInstrInfo, NFC

[Ahmed Bougacha via llvm-commits]

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

Everything LGTM, but you might want to wait for a third pair of eyes.  A few inline comments.

Also, I see BPF doesn't need any changes; can you confirm?


================
Comment at: include/llvm/Target/TargetInstrInfo.h:1197
@@ +1196,3 @@
+  /// Compute and return the latency of the given data dependent def and use
+  /// when the operand indices are already known. UseMI may be NULL for an
+  /// unknown use.
----------------
Might as well fix NULL -> null/nullptr

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:838
@@ -840,1 +837,3 @@
+    const TargetRegisterInfo *TRI, const AccessKind AccessToCheck = AK_All) {
   // We iterate backward starting \p To until we hit \p From
+  MachineBasicBlock::iterator B = To->getParent()->begin();
----------------
Move this comment to the for loop below?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:839
@@ -840,3 +838,3 @@
   // We iterate backward starting \p To until we hit \p From
-  MachineBasicBlock::iterator I = To, E = From, B = To->getParent()->begin();
+  MachineBasicBlock::iterator B = To->getParent()->begin();
 
----------------
and remove B and do

    if (To == To->getParent()->begin())

below?

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1132
@@ -1134,3 +1131,3 @@
   MI->setDesc(get(NewOpc));
-  CmpInstr->eraseFromParent();
-  bool succeeded = UpdateOperandRegClass(MI);
+  CmpInstr.eraseFromParent();
+  bool succeeded = UpdateOperandRegClass(*MI);
----------------
Here and elsewhere: It's not new and unlikely to become a problem, but I'm a tad worried about eraseFromParent() on references (though we usually do nothing to prevent reusing erased pointers either, at least we could, I guess).  WDYT?

================
Comment at: lib/Target/AMDGPU/R600ExpandSpecialInstrs.cpp:116
@@ -115,3 +115,3 @@
         }
         MI.eraseFromParent();
         continue;
----------------
Heh, I guess we already do that


http://reviews.llvm.org/D21726