[PATCH] D68209: [LiveDebugValues] Introduce entry values of unmodified params

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

After eyeballing the code in SelectionDAG that generates the additional DBG_VALUEs, it looks like disabling that code will generate a variety of other location regressions. LiveDebugValues doesn't always pick the "best" location when there are multiple options, see for example [0].

With that in mind, this code makes sense. My understanding of what's happening here is that whenever there's a register-copy-transfer, if it's from an entry-value-location then DebugEntryVals map records the transfer destination. Then, if another DBG_VALUE for the same variable is encountered specifying the same destination, it's interpreted as being caused by the register-copy, and so entry values can be propagated over it.

I think what's missing is handling of control flow branching / merging. As far as I can see, DebugEntryVals is a single map, that gets updated whenever a block is processed. If we had a standard diamond control flow like this:

  entry:
      setcc %somereg
      jnz block2
  block1:
      mov %rax, %rcx
      jmp exit
  block2:
      mov %rcx, %rdx
      jmp exit

Then the reverse-post-order order that LiveDebugValues follows will mean it visits blocks in the order {entry, block1, block2, exit} [1]. Isn't there a risk that changes to DebugEntryVals when processing block1, will be visible / will affect the handling of block2?

The rest of LiveDebugValues handles this by having a per-basic-block collection of in-locs / locations and updating them in the 'join' method. That would probably solve this potential issue too, but it add more complexity :(

[0] https://bugs.llvm.org/show_bug.cgi?id=42834
[1] although block1 and block2 might get flipped, I can't remember precisely.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:278
+                                     unsigned NewReg) {
+      VarLoc VL(MI, LS);
+      VL.Kind = EntryValueCopyRegKind;
----------------
It's probably worth asserting that you expect the incoming location to be a RegisterKind, should make it clearer for other readers.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:418
+  };
+  using DebugParamMap = SmallDenseMap<const DILocalVariable *, EntryValue>;
+
----------------
Can there be fragments of entry values? Typically a <DILocalVariable, InlinedAt, Fragment> tuple is needed to uniquely identify a variable location. I understand inlining doesn't make sense in the context of entry values, but if there can be fragments, then the map key here may need to account for them, now or in the future.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:720-721
 
+/// Try to salvage the debug entry value if encounter a new debug value
+/// describing the same parameter or stop tracking it.
+void LiveDebugValues::removeOrSalvageEntryValue(const MachineInstr &MI,
----------------
"entry value if _we_ encounter"


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

https://reviews.llvm.org/D68209