[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:2499
 
+static inline void populateRegisterAndInstrMapForDebugInstr(
+    Register Reg, SmallDenseMap<Register, MachineInstr *, 8> &RegisterMap,
----------------
nit: `static inline` is effectively equivalent to `static` and thus LLVM code usually uses `static` alone.

Given the ratio of the parameter list and the function body, maybe this makes more sense as a lambda anyway?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2543
+  // register that is the result of a load that is moved and the DBG_VALUE
+  // MachineInstr pointer that uses that virtual register.
+  SmallDenseMap<Register, MachineInstr *, 8> RegisterMap;
----------------
What happens if more than one DBG_VALUE use the same register?

```
int x, y;
x = y = *p;

-->

r0 = load ...
DBG_VALUE r0, 0, "x"
DBG_VALUE r0, 0, "y"
```


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2797
+      if (MI.isNonListDebugValue()) {
+        auto Op = MI.getOperand(0);
+        if (Op.isReg()) {
----------------
It looks like you use the same pattern of for_each_reg_operand_in_dbg_intrinsic 4 times. Maybe create 

```
forEachDbgRegOperand(MachineInstruction *MI, std::function<void(unsigned)> Fn)
```

helper function so you can write 

```
forEachDbgRegOperand(MI, [&](unsigned Reg) {
   populateRegisterAndInstrMapForDebugInstr(Reg, RegisterMap, InstrMap, &MI);
 });
```

?


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2803
+        }
+      } else if (MI.isDebugValueList()) {
+        for (unsigned I = 2; I < MI.getNumOperands(); I++) {
----------------
I think this can just be `else` with no condition.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2861
+      auto Opc = MI.getOpcode();
+      if (isLoadSingle(Opc)) {
+        auto Reg = MI.getOperand(0).getReg();
----------------
 if (!isLoadSingle(Opc))
  continue

?


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

https://reviews.llvm.org/D145168