[PATCH] D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations

Tue Jun 25 14:21:29 PDT 2019

aprantl added a comment.

In D56151#1557861 <https://reviews.llvm.org/D56151#1557861>, @jmorse wrote:

> Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?
>
> The patch effectively makes the codegen backend stricter about what DBG_VALUE insts it will accept, i.e. they have to refer to vregs that are alive. If it helps, this is already something that's enforced a few optimisation passes later in LiveDebugVariables [0], this patch just brings that check further forwards, due to the errors that can be introduced (described above). What's unfortunate is that this also prevents occasions where register coalescing re-enlivens DBG_VALUEs with a vreg that has the right value in it, occasions which can be generated due to limitations in SelectionDAG.
>
> This then becomes a trade-off (see immediately preceeding comment) of whether the problem is so bad that we should trade some lost locations for getting rid of placeDbgValues. IMHO: yes, because we're trading definitely-incorrect locations for some more being optimized-out, a net reduction in error, to me. Other opinions most welcome.
>
> [0] https://github.com/llvm/llvm-project/blob/b9f1e7b16ed2341e54b4e2033d111e7a2ca19b9a/llvm/lib/CodeGen/LiveDebugVariables.cpp#L624

I'm reading this as: "this patch make debug info more accurate by rejecting invalid DBG_VALUEs earlier on, but it overshoots the target a bit and also rejects a few(?) false positives". I support this approach, as we all want to move LLVM into the "more accurate" direction, assuming that the false positives are fixable and the ratio of rejected incorrect DBG_VALUEs to false positives is favorable.

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

https://reviews.llvm.org/D56151