Page MenuHomePhabricator

[DebugInfo][5/N] Recognise extra indirection in instruction-referencing for NRVO tests
ClosedPublic

Authored by jmorse on Mon, Nov 22, 5:32 AM.

Details

Summary

In some scenarios, usually involving NRVO, we can issue indirect DBG_VALUEs after SelectionDAG, even in instruction referencing mode (if the variable is an argument). If the corresponding argument value is spilt to the stack, then we have:

  • Indirection because it's on the stack,
  • Indirection because of the nature of the variable (i.e., it was created with indirection),

However InstrRefBasedLDV only emits one level of indirection. This patch adds the second, by adding an extra DW_OP_deref if necessary.

The two tests modified fail otherwise -- they feature some NRVO, and require two levels of indirection to be correct.

(An alternate solution would be to stop emitting DBG_VALUEs with indirection -- that'll probably work, but I think it should be part of a refactor. InstrRefBasedLDV is agnostic as to whether it's working on DBG_INSTR_REFs or DBG_VALUEs, I might have baked more assumptions into it).

Diff Detail

Event Timeline

jmorse created this revision.Mon, Nov 22, 5:32 AM
jmorse requested review of this revision.Mon, Nov 22, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 22, 5:32 AM

Would it make sense to have these tests run twice, once with experimental-debug-variable-locations being true and once with false? Otherwise it looks like they will no longer cover the normal DBG_VALUE case with this patch.

Would it make sense to have these tests run twice, once with experimental-debug-variable-locations being true and once with false? Otherwise it looks like they will no longer cover the normal DBG_VALUE case with this patch.

Sometimes yes, sometimes no, see discussion with Orlando here: https://reviews.llvm.org/D114362#3151143

IMO these tests would benefit little as they're really targetted at LiveDebugValues, and shouldn't really be .ll tests. On the other hand, given that there isn't actually a test difference between the two implementations, it won't hurt to add some extra RUN lines, WDYT?

IMO these tests would benefit little as they're really targetted at LiveDebugValues, and shouldn't really be .ll tests. On the other hand, given that there isn't actually a test difference between the two implementations, it won't hurt to add some extra RUN lines, WDYT?

If I understand right, these tests do cover behaviour in the VarLocBasedImpl that we want to maintain - without the extra run lines, someone could theoretically run through the VarLoc impl and delete the "add a deref when spilling already-indirect offsets" code, and that would no longer cause a test failure as of this patch. If I've understood incorrectly then the patch LGTM, otherwise I think it'd be good to have the extra run lines (and with those lines, LGTM).

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Nov 25, 1:44 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.