This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable
ClosedPublic

Authored by jmorse on Jan 10 2023, 7:24 AM.

Details

Summary

Currently, CodeGen passes like LiveDebugValues work out whether DBG_INSTR_REF or DBG_VALUE instructions are being used for variable locations by examining the current optimisation level, and other attributes of the function. The assumption is that, by examining the function and optimisation level, you can work out later which flavour was used. Unfortunately this isn't always true, as reported in https://github.com/llvm/llvm-project/issues/57997 , things like opt-bisect-limit break this assumption because the optimisation level can change. When it does, the result is usually assertion failures.

To fix this, make "instruction referencing" an attribute of a MachineFunction -- it's simplest to just store whether or not the function was generated with DBG_INSTR_REF or not in a Boolean, and use that flag as the source-of-truth, rather than try to hack around this any further. This patch adds that flag to the MachineFunction (and MIR) parser, and adds the flag to the existing instruction referencing tests. As a result of this, we can un-plumb some flags out of SelectionDAG/FastISel that effectively implement the same thing. It still makes sense for InstrEmitter to hold a local copy of the are-we-using-instr-ref-flag as it checks it quite frequently.

The test changes are unremarkable; however this change means we can't feed the same MIR into both flavours of LiveDebugValues and expect to get legitimate outputs. As a result, I've duplicated three tests, to have instr-ref and DBG_VALUE versions:

  • entry-value-of-modified-param.mir
  • kill-entry-value-after-diamond-bbs.mir
  • live-debug-values-bad-transfer.mir

Additionally there are two tests where I've just deleted the RUN lines for DBG_VALUE-behaviours and not duplicated the test:

  • single-assign-propagation.mir: this test is checking that a performance-optimisation in InstrRefBasedLDV actually behaves correctly, there's nothing interesting about the way it passes through VarLocsBasedLDV. The DBG_VALUE-based check lines were only in there to provide a comparison with how VarLocBasedLDV behaved, I don't believe we actually need to be testing them.
  • stack-coloring-dbg-phi.mir: this test exists because stack coloring changed codegen when it saw DBG_PHI instructions, it doesn't actually need a DBG_VALUE run line.

(retitled as this got submitted 5 seconds too early X_X)

Diff Detail

Event Timeline

jmorse created this revision.Jan 10 2023, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 7:24 AM
jmorse requested review of this revision.Jan 10 2023, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2023, 7:24 AM
jmorse retitled this revision from [DebugInfo] Make instr-ref/DBG_VALUE mode of a MachineFunction to [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable.Jan 10 2023, 7:26 AM
jmorse edited the summary of this revision. (Show Details)
Orlando accepted this revision.Jan 11 2023, 12:45 AM

Looking at other MF flags this seems reasonable. LGTM plus some nits.

llvm/lib/CodeGen/MIRPrinter.cpp
217

nit: I think it would make sense to move this to line 203, underneath YamlMF.HasEHFunclets = MF.hasEHFunclets();

llvm/test/DebugInfo/MIR/InstrRef/stack-coloring-dbg-phi.mir
6–11

nit: out of date comment

llvm/test/DebugInfo/MIR/X86/entry-value-of-modified-param.mir
27–28

Possibly worth either deleting the VARLOCLDV check and/or mentioning in both tests that they're clones other than the RUN lines (same for other duplicated tests). YMMV.

This revision is now accepted and ready to land.Jan 11 2023, 12:45 AM
jmorse updated this revision to Diff 490153.Jan 18 2023, 7:33 AM
jmorse marked 3 inline comments as done.

Apply review comments; I've also had an attack of conscience, and added some more check lines:

  • llvm/test/DebugInfo/x86/instr-ref-flag.ll -- check that the MIR function key gets produced with true or false values, according to the command line flags,
  • llvm/test/DebugInfo/MIR/InstrRef/instr-ref-roundtrip.mir -- check that the flag makes its way from the input to the output.

Still LGTM

llvm/test/DebugInfo/MIR/X86/live-debug-values-bad-transfer.mir
10

smallest of nits: missing full stop (only mentioning as it's inconsistent with the other changes!).

This revision was landed with ongoing or failed builds.Jan 20 2023, 6:47 AM
This revision was automatically updated to reflect the committed changes.