[PATCH] D123394: [CodeGen] Late cleanup of redundant address/immediate definitions.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:182
+    MachineBasicBlock *FirstPred = *MBB->pred_begin();
+    for (auto I : RegDefs[FirstPred->getNumber()]) {
+      Register Reg = I.first;
----------------
Could use structured bindings: `for (auto [Reg, DefMI] : ...)`


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:185
+      MachineInstr *DefMI = I.second;
+      if (llvm::all_of(MBB->predecessors(), [&](const MachineBasicBlock *Pred) {
+            if (Pred == FirstPred)
----------------
Could use `all_of(drop_begin(MBB->predecessors()), ...)`?


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:193-194
+        MBBDefs[Reg] = DefMI;
+        LLVM_DEBUG(dbgs() << "Reusable instruction from pred(s): in MBB#"
+                          << MBB->getNumber() << ":  " << *DefMI;);
+      }
----------------
Use `<< printMBBReference(*MBB) <<` instead of rolling your own MBB#number syntax, here and elsewhere.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:203-204
+  Register FrameReg = TRI->getFrameRegister(*MF);
+  for (MachineBasicBlock::iterator I = MBB->begin(); I != MBB->end();) {
+    MachineInstr *MI = &*(I++);
+
----------------
This looks like `for (auto &MI : make_early_inc_range(MBB)))`


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:206
+
+    // Clear map if FrameReg is modified.
+    if (MI->modifiesRegister(FrameReg, TRI)) {
----------------
Comment should explain //why// rather than //what//.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123394/new/

https://reviews.llvm.org/D123394