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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:47
+  // Data structures to map regs to their definitions per MBB.
+  using Reg2DefMap = std::map<Register, MachineInstr*>;
+  using MBB2RegDefsMap = std::map<MachineBasicBlock *, Reg2DefMap>;
----------------
Can these be DenseMaps?


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:52
+  // Set of visited MBBs.
+  MBBSet Visited;
+
----------------
Every MachineBasicBlock has a number assigned to it. This could possibly be a BitVector using the basic block numbering. See `MachineBasicBlock::getNumber()` and `MachineFunction::getNumBlockIDs`

You might also be able to use the numbering instead of a map for MBB2RegDefsMap. But probably depends on how spare that map is.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:138
+                             const TargetRegisterInfo *TRI) {
+  Visited_preds.insert(MBB);
+  while (I != MBB->begin()) {
----------------
No underscores in variable names. Use VisitedPreds


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:155
+    MBB->addLiveIn(Reg);
+  assert(MBB->pred_size() && "Predecessor def not found!");
+  for (MachineBasicBlock *Pred : MBB->predecessors())
----------------
!pred_empty


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:206
+  // Find reusable definitions in the predecessor(s).
+  if (MBB->pred_size() > 0) {
+    MachineBasicBlock *FirstPred = *MBB->pred_begin();
----------------
!MBB->pred_empty()?


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:222
+      }
+      if (AllSame) {
+        MBBDefs[Reg] = DefMI;
----------------
Could this use llvm::all_of with a lambda?


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:281
+
+  std::list<MachineBasicBlock *> Worklist;
+  Worklist.push_back(PendingMBB);
----------------
Would std::queue be better here?


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:283
+  Worklist.push_back(PendingMBB);
+  while (Worklist.size()) {
+    MachineBasicBlock *CurrMBB = Worklist.front();
----------------
!Worklist.empty()?


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

https://reviews.llvm.org/D123394