This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][4/N] Adjust x86 location-list tests for instruction referencing
ClosedPublic

Authored by jmorse on Nov 22 2021, 5:10 AM.

Details

Summary

This patch updates location lists in various x86 tests to reflect what instruction referencing produces. There are two flavours of change:

  • Not following a register copy immediately, because instruction referencing can make some slightly smarter decisions,
  • Extended ranges, due to having additional information.

The register changes aren't that interesting, it's just a choice between equally legitimate registers that instr-ref does differently. The extended ranges are largely due to following stack restores better.

Diff Detail

Event Timeline

jmorse created this revision.Nov 22 2021, 5:10 AM
jmorse requested review of this revision.Nov 22 2021, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2021, 5:10 AM

Test changes LGTM (just one non-blocking inline question), but wouldn't we want to preserve the -experimental-debug-variable-locations=false coverage that these tests are currently (pre-patch) providing too?

llvm/test/DebugInfo/COFF/register-variables.ll
48–50

Non-blocking question: why do we get c <- undef here? The non-instr-ref location c <- $eax looks correct AFAICT.

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-4.ll
9

Nice!

Orlando wrote:

wouldn't we want to preserve the -experimental-debug-variable-locations=false coverage that these tests are currently (pre-patch) providing

Yes and no; I think it's a question of what parts of the codegen pipeline are being tested. Most of these tests are checking parts of the DWARF emitter so that, for example, basic block sections have location lists divided on block boundaries (I think that's what it's doing?). Which isn't something affected by instruction referencing at all. In an ideal world these things would be MIR tests feeding into the DWARF emitter, however I suspect they were written before MIR existed / was good enough.

sdag-combine.ll is testing something that's likely to diverge between instr-ref and DBG_VALUEs, I guess that part does need duplicate test coverage.

llvm/test/DebugInfo/COFF/register-variables.ll
48–50

Looking at the IR, there's some tail-folding happening, where two instances of:

dbg.value
tail call void @putint(

on either side of a branch get merged together. In DBG_VALUE mode, I believe this works by coincidence: the tails get merged, debug instructions ignored, and the variable location preserved because one code sequence is spliced into the common tail. If there was only a dbg.value on one side, we'd end up with a false location.

With instruction referencing there's still one DBG_INSTR_REF preserved, but LiveDebugValues can tell that it isn't dominated by the definition it's referring to, so drops the location.

IMO this is a good demonstration that instruction referencing soundly drops questionable locations; exactly how to fix it is a wider question, and probably needs a little more instrumentation. I'll fold a note in remarking on the limitation.

Orlando accepted this revision.Nov 24 2021, 4:19 AM

Ok, LGTM with the duplicated test and folded-in limitation remark. Thanks!

llvm/test/DebugInfo/COFF/register-variables.ll
48–50

Aha, that is quite interesting. Thanks for the explanation.

This revision is now accepted and ready to land.Nov 24 2021, 4:19 AM
This revision was landed with ongoing or failed builds.Nov 24 2021, 4:31 AM
This revision was automatically updated to reflect the committed changes.