This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Allow DBG_VALUE to use undefined vregs before LiveDebugValues
ClosedPublic

Authored by jackoalan on Oct 29 2021, 3:53 PM.

Details

Summary

Expanding on D109750.

Since DBG_VALUE instructions have final register validity determined in
LDVImpl::handleDebugValue, there is no apparent reason to immediately prune
unused register operands as their defs are erased. Consequently, this renders
MachineInstr::eraseFromParentAndMarkDBGValuesForRemoval moot; gaining a
substantial performance improvement.

The only necessary changes involve making relevant passes consider invalid
DBG_VALUE vregs uses as valid.

Diff Detail

Event Timeline

jackoalan created this revision.Oct 29 2021, 3:53 PM
jackoalan requested review of this revision.Oct 29 2021, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2021, 3:53 PM
jackoalan edited the summary of this revision. (Show Details)Oct 29 2021, 4:02 PM
jackoalan edited the summary of this revision. (Show Details)Oct 29 2021, 4:03 PM
jackoalan updated this revision to Diff 383533.Oct 29 2021, 4:34 PM

Update comment

No final verdict for this change yet. But you should try to use a more specific wording than "invalid vreg"; is it a vreg without definition?

Before we weaken the verifier, do we have an idea how much benefit we get from allowing these half-initialize vregs? Is searching for these DBG_VALUE at the end of GlobalISel and remove them there an option?

Specifically check for vregs without defs when suppressing verification errors.

jackoalan added a comment.EditedNov 20 2021, 10:29 AM

Before we weaken the verifier

The weakening conditions are now much more targeted to this specific case.

do we have an idea how much benefit we get from allowing these half-initialize vregs?

In terms of benefits, D109750 mentions a 0.5% codegen time reduction, but its effects are limited to the legalizer. This goes further by replacing all calls to eraseFromParentAndMarkDBGValuesForRemoval with eraseFromParent.

Is searching for these DBG_VALUE at the end of GlobalISel and remove them there an option?

The issue here is determining where the barrier for disallowing these undefined vregs should be. LiveDebugVariables naturally discards these particular uses, so this is sufficient based on my knowledge.

MatzeB requested changes to this revision.Nov 22 2021, 11:23 AM

Requesting changes for the missing MIR serialization of the new property.

The issue here is determining where the barrier for disallowing these undefined vregs should be. LiveDebugVariables naturally discards these particular uses, so this is sufficient based on my knowledge.

Well to me this feels like we are making the intermediate representation more complicated (I consider it more complicated because DBG_VALUE instructions are special with lifetime rules now) for a very small gain.

That said I don't feel strongly about this. If others think this is benefitial then we can go ahead.

llvm/include/llvm/CodeGen/MachineFunction.h
166 ↗(On Diff #388711)

New properties must be added to MIRYamlMapping.h / MIRParser.cpp

This revision now requires changes to proceed.Nov 22 2021, 11:23 AM
MatzeB added inline comments.Nov 22 2021, 11:25 AM
llvm/include/llvm/CodeGen/MachineFunction.h
153–155 ↗(On Diff #388711)

Please document the new property here

166 ↗(On Diff #388711)

FWIW: It seems there are others missing as well already :-(

That said I don't feel strongly about this. If others think this is benefitial then we can go ahead.

I agree this change should be open for comment. If it is decided to not be worth it, D109750 should be reverted (the test case provided here fails due to the changes made there).

Requesting changes for the missing MIR serialization of the new property.

The issue here is determining where the barrier for disallowing these undefined vregs should be. LiveDebugVariables naturally discards these particular uses, so this is sufficient based on my knowledge.

Well to me this feels like we are making the intermediate representation more complicated (I consider it more complicated because DBG_VALUE instructions are special with lifetime rules now) for a very small gain.

That said I don't feel strongly about this. If others think this is benefitial then we can go ahead.

We haven’t changed the meaning of the MIR, we just clarified the existing semantics on the RFC thread. There’s no middle ground here as far as I can see, either it’s valid to have undefined uses or not. If it is, then we’re free to do anything that leaves them around.

Drive by comment,

Possibly relevant to the interests of this thread, SelectionDAG will already produce half-formed DBG_VALUEs in it's current implementation. The regression test in the recently landed rGb8f68ad9cdb, if run with

llc llvm/test/DebugInfo/X86/instr-ref-sdag-empty-vreg.ll -o - -stop-before=finalize-isel -experimental-debug-variable-locations=false

produces:

body:             |
  bb.0.cond.false.i:
    successors: %bb.1(0x80000000)


  bb.1._ZN4Vec39normalizeEv.exit:
    DBG_VALUE %1:fr32, $noreg, !11, !DIExpression(), debug-location !12
    RET 0, debug-location !12

due to the argument value being optimised out late, leaving a DBG_VALUE referring to an undefined vreg. So I suppose you could say the approach discussed here has precedent.

I wasn't aware that SelectionDAG behaved like this until quite recently. NB: the -experimental...=false flag is the current llc default, I've added it because it might change in the next week or two.

jackoalan updated this revision to Diff 389827.EditedNov 25 2021, 10:20 AM
  • Rename DebugValuesAllocated -> TracksDebugUserValues
  • Documenting comment for TracksDebugUserValues
  • MIR key for TracksDebugUserValues
  • TracksDebugUserValues tests for MachineVerifier
  • LiveDebugValues test to ensure undefined debug uses delete DBG_VALUE instructions
jackoalan retitled this revision from [GlobalISel] Allow DBG_VALUE to use invalid vregs throughout pipeline to [GlobalISel] Allow DBG_VALUE to use undefined vregs before LiveDebugValues.Nov 25 2021, 10:21 AM

Clarify TracksDebugUserValues documenting comment.

jackoalan marked 3 inline comments as done.Nov 25 2021, 10:29 AM
MatzeB accepted this revision.Dec 2 2021, 3:35 PM

Technical side seems fine. LGTM

llvm/include/llvm/CodeGen/MachineFunction.h
155–158 ↗(On Diff #389837)

I'd primary describe the properties as constraints on the MIR. (And only mention passes secondary to motivate things):

TracksDebugUserValues: Without this property enabled, debug instructions such as DBG_VALUE are allowed to reference virtual registers even if those registers do not have a definition. With the property enabled virtual registers must only be used if they have a definition. This property allows earlier passes in the pipeline to skip updates of `DBG_VALUE` instructions to save compile time.
This revision is now accepted and ready to land.Dec 2 2021, 3:35 PM
jackoalan updated this revision to Diff 391630.EditedDec 3 2021, 7:09 AM

Rebase, Change TracksDebugUserValues comment per review request

Thanks everyone for the comments. May I have somebody commit this for me?
--author="Jack Andersen <jackoalan@gmail.com>"

bogner added a subscriber: bogner.Dec 17 2021, 1:35 PM
bogner added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
1995

Is this check in the right place? I'm seeing "Generic virtual register use cannot be undef" (just above) errors after this change on an out of tree target.

jackoalan added inline comments.Dec 17 2021, 2:10 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1995

Does the error occur after the Debug Variable Analysis pass on a debug instruction?

If possible, it would be really helpful if a test could be created that triggers the error on the main branch.

bogner added inline comments.Dec 17 2021, 2:23 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1995

The error occurs before the Debug Variable Analysis pass runs when running llc with -verify-machineinstrs, since that runs the machine verifier between each pass. I can try to see if I can find a reproducer for an in-tree backend, but I figured I'd sanity check this first, given that the comments says "Debug value instruction is permitted to use undefined vregs.", but the check that rejects undef vregs happens right before this.

jackoalan added inline comments.Dec 17 2021, 2:43 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1995

Okay, I see what you're referring to.

The purpose of this added if block is to weaken the verifier just for DBG_VALUE register uses; making it convenient to erase instructions that define these values but then not explicitly undef-ing them on the DBG_VALUE use (i.e. just a dangling vreg). In this line of thinking, it would not be necessary to also weaken MO->isUndef (which only tests for explicit undefs).

Does the error contain a dump of the instruction? If it is indeed a DBG_VALUE or DBG_VALUE_LIST instruction, then MO->isUndef() should be weakened as well if it is determined there is no other root cause. If it is not a DBG_VALUE instruction, then it would not be applicable and the root cause must be elsewhere.

A triggering test would be the best way to determine this collaboratively.

bogner added inline comments.Dec 17 2021, 2:48 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1995

The errors look something like this:

*** Bad machine code: Generic virtual register use cannot be undef ***
- function:    xyz
- basic block: %bb.4 while.end64.i.i (0x7fdf95dc8c48)
- instruction: DBG_VALUE undef %2423:gpr, $noreg, !"x", !DIExpression(DW_OP_constu, 0, DW_OP_swap, DW_OP_xderef), debug-location !3302; /...:184:30 @[ program_source:273:69 @[ program_source:0 ] ] line no:184
- operand 0:   undef %2423:grp

Let me see if I can reduce this test to something shareable.

jackoalan added inline comments.Dec 21 2021, 9:03 AM
llvm/lib/CodeGen/MachineVerifier.cpp
1995

Have you tried using bugpoint to reduce your crashing function to something you can share? You may have to anonymize identifiers, but it usually does a good job minimizing the IR to just the blocks and instructions necessary to crash.

I’d be happy to weaken that undef test for DBG_VALUE, but It’d be ideal to justify it with a test if that change is made.