[PATCH] D145168: Move DBG_VALUE's that depend on loads to after a load if the load is moved due to the pre register allocation ld/st optimization pass

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2480
+            if (isLd) {
+              // We are moving loads, so we need to record the register that the
+              // load writes to, for DBG_VALUE moving in the future.
----------------
This comment doesn't really tell the reader much that isn't already there in the code.
Shorter version:
```
// Populate RegisterMap with all Registers defined by loads.
```



================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2519
+    MachineInstr *DbgValueListInstr, MachineInstr *InstrToReplace) {
+  for (unsigned I = 2; I < DbgValueListInstr->getNumOperands(); I++) {
+    auto &Op = DbgValueListInstr->getOperand(I);
----------------
Why aren't you using `forEachDbgRegOperand()` here?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2522
+    if (Op.isReg()) {
+      // RegisterMap[Op.getReg()].push_back(DbgValueListInstr);
+      auto RegIt = RegisterMap.find(Op.getReg());
----------------
Commented out code.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2792
+      if (RegisterMap.find(Reg) != RegisterMap.end()) {
+        RegisterMap[Reg].push_back(&MI);
+        InstrMap[&MI].push_back(Reg);
----------------
You're looking up Reg twice here.
```
auto &Entry = RegisterMap.find(Reg);
if (Entry != RegisterMap.end())
  Entry.push_back(&MI);
```


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2826
+          // RegisterMap that describes the DbgVar.
+          for (auto Reg : RegVec) {
+            auto RegIt = RegisterMap.find(Reg);
----------------
auto &?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2831
+            auto &InstrVec = RegIt->getSecond();
+            for (auto *It = InstrVec.begin(); It != InstrVec.end();) {
+              auto *I = *It;
----------------
does this loop get shorter if you implement it in terms of std::remove_if?

https://en.cppreference.com/w/cpp/algorithm/remove


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2844
+                               [&](MachineOperand &Op) { Op.setReg(0); });
+        } else {
+          auto &Op = Instr->getOperand(0);
----------------
Maybe comment here what the else branch means, since it's far below from the condition.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2848
+          if (Op.isImm()) {
+            unsigned Opcode = Instr->getOpcode();
+            auto BI =
----------------
Would `Instr->getOperand(i).setReg(0)` work here and be shorter?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2884
+                                    DbgInstr->getDebugExpression(),
+                                    DbgInstr->getDebugLoc()->getInlinedAt());
+        auto DbgIt = DbgValueSinkCandidates.find(DbgVar);
----------------
This is the second time you use this pattern. Maybe add a constructor to DebugVariable that takes a MachineInstr as parameter, or a static DebugVariable::createFromDebugValue() method?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2890
+        if (DbgIt != DbgValueSinkCandidates.end())
+          DbgValueSinkCandidates.erase(DbgIt);
+        // Zero out original dbg instr
----------------
Is it really necessary to look it up before erasing it, or can you call straight into erase, too?


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

https://reviews.llvm.org/D145168