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

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a reviewer: MatzeB.
MatzeB added a comment.

I like these changes, thanks for doing all the work.

- As expected not much to see in thediff, it is a mechanical change (with some code reformating).
- This will bring some churn to people backporting patches and exploring version history but that is of course no good reason to stop this.
- I assume we have a ubsanbot that catches references pointing to nullptr.

LGTM


================
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);
----------------
ab wrote:
> 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?
I don't see a problem here. It would probably feel more natural as ˋMI.getParent().erase(MI);ˋ but that is not how we do things today...

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1091
@@ -1090,3 +1090,3 @@
                      const AArch64InstrInfo *TII) {
-  for (auto &MIb : MemInsns)
-    if (mayAlias(MIa, MIb, TII))
+  for (MachineInstr *MIb : MemInsns)
+    if (mayAlias(MIa, *MIb, TII))
----------------
Could have been done separately, but +1 to not using auto when it is not extremely obvious.

================
Comment at: lib/Target/AArch64/AArch64SchedCyclone.td:111
@@ -110,3 +110,3 @@
 // AND Rd, Rzr, #imm
-def WriteZPred : SchedPredicate<[{TII->isGPRZero(MI)}]>;
+def WriteZPred : SchedPredicate<[{TII->isGPRZero(*MI)}]>;
 def WriteImmZ  : SchedWriteVariant<[
----------------
We should change the tablegen generated code to use MachineInstr&, but that is for another patch.


http://reviews.llvm.org/D21726