[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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This is a good improvement, many thanks for working in this area. The overall approach is good, I've got a couple of high level comments:

- I believe you'll need to erase elements from `LocalVarMap` once a DBG_VALUE is sunk -- if a later, unrelated DBG_VALUE for the same variable is encountered in the same block, then there's no need terminate the earlier sunk DBG_VALUE,
- You should use the `DebugVariable` class to identify variables, as that combines inlining and fragment information into a single object. Using just DILocalVariable risks duplicate instances of the same variable from different inlining contexts being merged.

Finally, I think that rather than just sinking the DBG_VALUE that used to refer to a load, it might be more correct to duplicate the DBG_VALUE to below the load, and mark the old one as undef/$noreg. Consider:

  %0 = load, !dbg !0
  DBG_VALUE %0
  %1 = foo, !dbg !1
  %2 = bar, !dbg !2
  %3 = baz, !dbg !3

If we sink the load to below baz, and sink the DBG_VALUE too, we get:

  %1 = foo, !dbg !1
  %2 = bar, !dbg !2
  %3 = baz, !dbg !3
  %0 = load, !dbg !0
  DBG_VALUE %0

Which means that whatever value the variable used to have at the start of the block extends over the "foo", "bar" and "baz" instructions, which risks giving a misleading variable value on those line numbers. It might be better to produce:

  DBG_VALUE $noreg
  %1 = foo, !dbg !1
  %2 = bar, !dbg !2
  %3 = baz, !dbg !3
  %0 = load, !dbg !0
  DBG_VALUE %0

instead.



================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2613-2614
+  // used by the DBG_VAL that comes after it, is moved to below that DBG_VAL,
+  // this results in the debug info for that local variable to be lost after
+  // this pass. The code below address this issue by moving any DBG_VALs that
+  // depend on such loads which have been moved to below those loads.
----------------
IMHO, describing this as a "use-before-def" would indicate to the reader exactly why the debug-info is being lost after the pass.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2764
+    MachineInstr &MI = *MBBI;
+    if (MI.isDebugInstr() && MI.isDebugValueLike()) {
+      auto Op = MI.getOperand(0);
----------------
aprantl wrote:
> Are there isDebugInstr() that are not isDebugValueLike()?
> 
> 
DBG_LABEL, I believe -- isDebugValueLike should be sufficient.


================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:2767-2770
+      // TODO: This should not happen, have to fix the MIR verifier to check for
+      // such instances and fix them.
+      if (!DbgVar)
+        continue;
----------------
I think there are some tests out there that deliberately give an empty variable to test things like the the MIR parser. IMHO, asserting here is acceptable as any "real" DBG_VALUE must have a variable.


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

https://reviews.llvm.org/D145168