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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Hi,

Djordje wrote:

> Nevertheless, there are many cases where (due to various optimizations) a parameter value is only moved (copied) around, from one register to another one. We should recognize such situation and keep tracking of entry value of such parameter, since the value actually has not changed.

A large number of these cases Should (TM) have gone away with D67393 <https://reviews.llvm.org/D67393>, as any transfer DBG_VALUEs created by LiveDebugValues don't enter the instruction stream until after LiveDebugValues completes. In fact, applying the two parent patches and the test from this change to master (which I've done in this tree here [0]), the test already passes for me. (There's a decent chance I mucked something up though).

However, the test case demonstrates something I've found odd in LLVM instruction selection: it creates _two_ DBG_VALUEs for arguments. The code that does it is here [1], dates back to about 2010, and sounds like it tries to propagate variable locations in a way that is now LiveDebugValues domain. If it didn't create an extra DBG_VALUE for every argument, we might not need special handling for extra DBG_VALUEs here. Removing the code in [1] makes very little difference to variable-location coverage statistics, but does trip 12 tests.

I don't have any specific feedback on this patch, but I get the feeling that it might not be necessary if we eliminated [1]; I don't suppose anyone remembers what it's there for?

(Sorry for driving this review in an unrelated direction, but it'd be nice if we could design-out this problem).

[0] https://github.com/jmorse/llvm-project/commits/d68209_experiment
[1] https://github.com/llvm/llvm-project/blob/f24ac13aaae63d92317dac839ce57857a7b444dc/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp#L587



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1449
         !DebugEntryVals.count(MI.getDebugVariable()) &&
         MI.getDebugExpression()->getNumElements() == 0)
+      DebugEntryVals[MI.getDebugVariable()] = EntryValue{&MI, nullptr};
----------------
Note that this hunk didn't apply cleanly for me; on master this line (1319/1449) reads

    !MI.getDebugExpression()->isFragment())


================
Comment at: llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir:19-22
+# CHECK: DBG_VALUE $edi, $noreg, !16, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK: DBG_VALUE $edx, $noreg, !18, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK: DBG_VALUE $edi, $noreg, !16, !DIExpression(DW_OP_LLVM_entry_value, 1), debug-location {{.*}}
+# CHECK-NOT: DBG_VALUE $esi, $noreg, !17, !DIExpression(DW_OP_LLVM_entry_value, 1, DW_OP_plus_uconst, 7, DW_OP_stack_value), debug-location {{.*}}
----------------
Best to not bake in the metadata numbers directly, and instead to recognise the DILocalVariable records, capture their number, and check for DBG_VALUEs with those captured numbers.


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

https://reviews.llvm.org/D68209