[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

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

rastogishubham marked 5 inline comments as done.
rastogishubham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2499
 
+static inline void populateRegisterAndInstrMapForDebugInstr(
+    Register Reg, SmallDenseMap<Register, MachineInstr *, 8> &RegisterMap,
----------------
aprantl wrote:
> 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?
I am still not used to writing lambdas, so they don't come to mind as a first thing, but I think it makes a lot of sense here, I also replaced 
```
undefDbgValueList
```
with a lambda


================
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;
----------------
aprantl wrote:
> 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"
> ```
Good testcase, I have addressed that issue now.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2797
+      if (MI.isNonListDebugValue()) {
+        auto Op = MI.getOperand(0);
+        if (Op.isReg()) {
----------------
aprantl wrote:
> 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);
>  });
> ```
> 
> ?
This is a great suggestion, thank you. There are actually 6 places :)


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2861
+      auto Opc = MI.getOpcode();
+      if (isLoadSingle(Opc)) {
+        auto Reg = MI.getOperand(0).getReg();
----------------
aprantl wrote:
>  if (!isLoadSingle(Opc))
>   continue
> 
> ?
Ah yes, llvm likes its early exists, good catch.


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

https://reviews.llvm.org/D145168