This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][NFC] Prevent some tests from running in instruction-referencing mode
ClosedPublic

Authored by jmorse on Nov 4 2021, 8:53 AM.

Details

Summary

This patch is very simple: it adds -experimental-debug-variable-locations=false to ~100 tests. This patch is just making explicit what they're currently testing.

The reason why: shortly I'd like to RFC about turning on instruction referencing by default, a pre-requisite for which is that all the tests pass with instruction referencing turned on. Hence, making these ~100 tests explicitly not test instruction referencing.

Obviously, some of these tests cover important things that need to continue being tested with instruction referencing (there are about thirty SelectionDAG where-to-place-the-debug-instr tests), I've got a breakdown of them below. I'd prefer to mark all these things -experimental-debug-variable-locations=false and then update them with instruction referencing CHECKs later as that allows for switching the PS4 buildbot to instr-ref pretty much immediately and enables other people to run their own tests without noise.

The risk is that not all get converted and we lose test coverage in the long term; if that's too high a risk, I can do the test conversion first.

Test breakdown (deeply boring):

Tests that are examining something specific to DBG_VALUE-based variable tracking, usually that livedebugvariables behaves in a certain way. These are irrelevant to instruction referencing, and should be left testing DBG_VALUE-based tracking.

  • llvm/test/CodeGen/PowerPC/pr38899-split-register-at-spill.mir
  • llvm/test/DebugInfo/MIR/Mips/livedebugvars-stop-trimming-loc.mir
  • llvm/test/DebugInfo/MIR/X86/kill-after-spill.mir
  • llvm/test/DebugInfo/MIR/X86/live-debug-vars-unused-arg-debugonly.mir
  • llvm/test/DebugInfo/MIR/X86/live-debug-vars-unused-arg.mir
  • llvm/test/DebugInfo/MIR/X86/livedebugvars-crossbb-interval.mir
  • llvm/test/DebugInfo/X86/live-debug-vars-discard-invalid.mir
  • llvm/test/DebugInfo/X86/live-debug-vars-intervals.mir
  • llvm/test/DebugInfo/X86/pr34545.ll
  • llvm/test/DebugInfo/X86/sdag-combine.ll

These all contain CHECKs for DBG_VALUEs, but are testing something important (like SelectionDAGs placement of them), that we do the right thing for big-endian stack locations, and so forth. This is the set that should definitely be updated to have instruction-referencing CHECKs.

  • llvm/test/CodeGen/ARM/dbg-range-extension.mir
  • llvm/test/CodeGen/X86/dbg-value-superreg-copy.mir
  • llvm/test/CodeGen/X86/fast-regalloc-live-out-debug-values.mir (regalloc misery)
  • llvm/test/DebugInfo/AArch64/compiler-gen-bbs-livedebugvalues.mir
  • llvm/test/DebugInfo/MIR/X86/backup-entry-values-usage.mir
  • llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir (has instrref verion?)
  • llvm/test/DebugInfo/MIR/X86/kill-entry-value-after-diamond-bbs.mir (same)
  • llvm/test/DebugInfo/MIR/X86/live-debug-values-bad-transfer.mir (same)
  • llvm/test/DebugInfo/MIR/X86/live-debug-values-cutoffs.mir (same)
  • llvm/test/DebugInfo/MIR/X86/live-debug-values-fragments.mir
  • llvm/test/DebugInfo/MIR/X86/live-debug-values-restore-collide.mir (distinct stack slots)
  • llvm/test/DebugInfo/MIR/X86/live-debug-values-restore.mir
  • llvm/test/CodeGen/AArch64/wrong_debug_loc_after_regalloc.ll (dbg_ref plcmnt)
  • llvm/test/CodeGen/AMDGPU/debug-value.ll (double emission test?)
  • llvm/test/CodeGen/AMDGPU/debug-value2.ll
  • llvm/test/CodeGen/AMDGPU/post-ra-soft-clause-dbg-info.ll (gnochange)
  • llvm/test/CodeGen/AMDGPU/ptr-arg-dbg-value.ll (sdag test)
  • llvm/test/CodeGen/ARM/debuginfo-split-carryexpr.ll (loc-should-be-dropped)
  • llvm/test/CodeGen/ARM/fragmented-args-multiple-regs.ll (sdag test)
  • llvm/test/CodeGen/PowerPC/debuginfo-split-int.ll (endianness test)
  • llvm/test/DebugInfo/ARM/sdag-split-arg1.ll
  • llvm/test/DebugInfo/Generic/linear-dbg-value.ll (sdag test)
  • llvm/test/DebugInfo/MSP430/sdagsplit-1.ll (splitting sdag test)
  • llvm/test/DebugInfo/NVPTX/dbg-value-const-byref.ll
  • llvm/test/DebugInfo/PowerPC/live-debug-vars-subreg-offset.ll (big endian stack)
  • llvm/test/DebugInfo/X86/dbg-value-arg-movement.ll
  • llvm/test/DebugInfo/X86/dbg-value-funcarg.ll
  • llvm/test/DebugInfo/X86/dbg-value-funcarg2.ll
  • llvm/test/DebugInfo/X86/dbg-value-funcarg3.ll
  • llvm/test/DebugInfo/X86/float_const_loclist.ll
  • llvm/test/DebugInfo/X86/pr40427.ll
  • llvm/test/DebugInfo/X86/sdag-dangling-dbgvalue.ll
  • llvm/test/DebugInfo/X86/sdag-dbgvalue-phi-use-1.ll
  • llvm/test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll
  • llvm/test/DebugInfo/X86/sdag-dbgvalue-phi-use-3.ll
  • llvm/test/DebugInfo/X86/sdag-dbgvalue-phi-use-4.ll
  • llvm/test/DebugInfo/X86/sdag-dbgvalue-ssareg.ll
  • llvm/test/DebugInfo/X86/sdag-ir-salvage.ll
  • llvm/test/DebugInfo/X86/sdag-salvage-add.ll
  • llvm/test/DebugInfo/X86/sdag-split-arg.ll
  • llvm/test/DebugInfo/X86/sdag-transfer-dbgvalue.ll
  • llvm/test/DebugInfo/X86/sdagsplit-1.ll

Instruction referencing produces different location lists, and these tests encode specific expectations about where ranges start and end. They can just be updated to have instruction referencing versions:

  • llvm/test/CodeGen/ARM/debug-info-sreg2.ll
  • llvm/test/CodeGen/X86/2010-05-26-DotDebugLoc.ll
  • llvm/test/DebugInfo/COFF/fpo-stack-protect.ll
  • llvm/test/DebugInfo/COFF/pieces.ll
  • llvm/test/DebugInfo/COFF/register-variables.ll
  • llvm/test/DebugInfo/NVPTX/cu-range-hole.ll
  • llvm/test/DebugInfo/NVPTX/debug-info.ll
  • llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll (outp order ch)
  • llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-4.ll (")
  • llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-5.ll (")
  • llvm/test/DebugInfo/X86/constant-loclist.ll
  • llvm/test/DebugInfo/X86/dbg-addr-dse.ll (un-necessary entry?)
  • llvm/test/DebugInfo/X86/live-debug-values.ll (Doesn't anticipate entry vals)
  • llvm/test/DebugInfo/X86/live-debug-variables.ll (wants name change too!)
  • llvm/test/DebugInfo/X86/pieces-3.ll
  • llvm/test/DebugInfo/X86/pieces-4.ll (bakes loclist addr into test)
  • llvm/test/DebugInfo/X86/spill-nospill.ll
  • llvm/test/tools/llvm-locstats/locstats.ll
  • llvm/test/DebugInfo/COFF/types-array-advanced.ll (change in variable order)

Variadic variable locations: I haven't implemented this for instruction referencing yet, it's probably a week or two of work:

  • llvm/test/DebugInfo/MIR/X86/dvl-livedebugvalues-clobber.mir
  • llvm/test/DebugInfo/MIR/X86/dvl-livedebugvalues-join.mir
  • llvm/test/DebugInfo/MIR/X86/dvl-livedebugvalues-movements.mir
  • llvm/test/DebugInfo/MIR/X86/dvl-livedebugvalues-spillrestore.mir
  • llvm/test/DebugInfo/X86/dbg-val-list-dangling.ll

Testing of global-isel / fast-isel that only generate DBG_VALUEs right now (instruction referencing doesn't expected to see DBG_VALUEs of vregs)

  • llvm/test/CodeGen/AArch64/GlobalISel/debug-insts.ll
  • llvm/test/CodeGen/AArch64/fast-isel-dbg.ll
  • llvm/test/DebugInfo/AArch64/pr40709.ll

Textual printing of DBG_VALUEs isn't quite as expected with instruction referencing:

  • llvm/test/CodeGen/ARM/2010-08-04-StackVariable.ll
  • llvm/test/CodeGen/ARM/debug-info-branch-folding.ll
  • llvm/test/CodeGen/X86/debug-loclists.ll

Webassembly: I don't know how this works, and it probably won't move to using instruction referencing any time soon anyway,

  • llvm/test/CodeGen/WebAssembly/cfg-stackify-dbg-skip.ll
  • llvm/test/CodeGen/WebAssembly/dbgvalue.ll
  • llvm/test/CodeGen/WebAssembly/stackified-debug.ll
  • llvm/test/DebugInfo/WebAssembly/dbg-value-dwarfdump.ll
  • llvm/test/DebugInfo/WebAssembly/dbg-value-live-interval.ll
  • llvm/test/DebugInfo/WebAssembly/dbg-value-move-2.ll
  • llvm/test/DebugInfo/WebAssembly/dbg-value-move.ll
  • llvm/test/DebugInfo/WebAssembly/dbg-value-ti.ll
  • llvm/test/MC/WebAssembly/debug-byval-struct.ll

More investigation needed as it wasn't immediately obvious what was wrong / different.

  • llvm/test/CodeGen/MIR/X86/diexpr-win32.mir
  • llvm/test/DebugInfo/MIR/X86/mlicm-hoist-post-regalloc.mir
  • llvm/test/CodeGen/ARM/debug-info-s16-reg.ll (drops locs?)
  • llvm/test/DebugInfo/ARM/partial-subreg.ll
  • llvm/test/DebugInfo/COFF/fpo-shrink-wrap.ll
  • llvm/test/DebugInfo/COFF/fpo-stack-protect.ll
  • llvm/test/DebugInfo/X86/dbg-addr.ll (knackered indirection?)
  • llvm/test/DebugInfo/X86/spill-indirect-nrvo.ll (knackered indirection?)
  • llvm/test/DebugInfo/X86/spill-nontrivial-param.ll (knackered indirection?)
  • llvm/test/CodeGen/AArch64/live-debugvalues-sve.mir
  • llvm/test/DebugInfo/COFF/fp-stack.ll <--- x87 stack is near-impossible to deal with
  • llvm/test/DebugInfo/X86/instr-ref-selectiondag.ll <--- is just incomplete
  • llvm/test/DebugInfo/X86/live-debug-values-remove-range.ll <--- to delete?
  • llvm/test/DebugInfo/X86/stack-value-dwarf2.ll <--- no location lists?

Diff Detail

Event Timeline

jmorse created this revision.Nov 4 2021, 8:53 AM
jmorse requested review of this revision.Nov 4 2021, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 8:53 AM
jmorse planned changes to this revision.Nov 4 2021, 9:10 AM

Hmmmm, and I see that because of all the different architecture debug-info tests I've touched, herald has subscribed lots of people to this (hi!). The intention with this feature is to turn it on for a couple of architectures first, then expand to everything else. With that in mind, I suppose it'd be unhelpful to change the tests for architectures I'm not immediately looking at: they _should_ continue to fail if they experience this new feature, so that they're not accidentally missed in the future.

I'll revise the patch to not touch them shortly.

The risk is that not all get converted and we lose test coverage in the long term; if that's too high a risk, I can do the test conversion first.

That seems to only be an issue if the defaults are changed before these tests are cleaned up - if having all these tests flagged this way makes it easier for local/branched (not changes to the actual LLVM github repo) that seems fine, and it's a matter of policing/ensuring the defaults aren't changed in-tree until all this is cleaned up/well understood. The flag name seems readily greppable, so I don't think that's a huge risk so long as everyone involved is aware of/remembers/ensures these are addressed before any mainline default change is made.

@jmorse Thanks a lot for this work! It seems that we are about to see this enabled by default not only for X86 - that is cool!

Regarding this change, overall it looks good to me. I am not sure what is the exact/official strategy inside LLVM community, but (I guess) if the test coverage isn't significantly broken it can be addressed as a future work by converting all the (interesting) tests into new MIR (with the new DBG instructions).

jmorse updated this revision to Diff 385884.Nov 9 2021, 10:40 AM

Update to remove non-x86 tests from the set labelled as "only test without instruction referencing". The non-x86 tests will fail if they're run with instruction referencing, which is the correct behaviour at this time.

What with this technically being NFC, without objection I'll land it tomorrow shortly before sending the RFC email out.

Regarding this change, overall it looks good to me. I am not sure what is the exact/official strategy inside LLVM community, but (I guess) if the test coverage isn't significantly broken it can be addressed as a future work by converting all the (interesting) tests into new MIR (with the new DBG instructions).

I'd really enjoy some of these .ll tests becoming MIR tests; it's a considerable effort though, as things like DebugInfo/X86/stack-value-dwarf2.ll have a single CHECK line and no context to explain the intention behind what they're checking.

Update to remove non-x86 tests from the set labelled as "only test without instruction referencing". The non-x86 tests will fail if they're run with instruction referencing, which is the correct behaviour at this time.

What with this technically being NFC, without objection I'll land it tomorrow shortly before sending the RFC email out.

Generally once something's sent for review it should get some approval/review rather than "landing if no one objects" (general community standards - as this can lead to folks sending something out for review and then submitting things without review only because review was slow, not because the patch didn't need review)

djtodoro accepted this revision.Nov 10 2021, 12:29 AM

lgtm, thanks!

This revision is now accepted and ready to land.Nov 10 2021, 12:29 AM
This revision was landed with ongoing or failed builds.Nov 17 2021, 3:51 AM
This revision was automatically updated to reflect the committed changes.