Page MenuHomePhabricator

[DebugInfo] Emit DBG_VALUE_LIST from ISel
ClosedPublic

Authored by StephenTozer on Sep 30 2020, 10:05 AM.

Details

Summary

This patch continues implementing the ISel support for multi-location-operand debug values by enabling the emission of DBG_VALUE_LIST for variadic SDDbgValues. This patch does not add support for processing dbg.value intrinsics; that is added in a separate patch in this stack.

This patch is relatively small, with most of the patch being the extraction of location operand emission from EmitDbgValue to AddDbgValueLocationOps, so that it can be used for both the variadic set of location ops in DBG_VALUE_LIST as well as the single location op in DBG_VALUE. Outside of that, the only behaviour change is that the scheduler has a lambda added, HasUnknownVReg, to prevent us from attempting to emit a DBG_VALUE_LIST before all of its used VRegs have become available.

Diff Detail

Event Timeline

StephenTozer created this revision.Sep 30 2020, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2020, 10:05 AM

Rebased onto recent master; no functional changes.

aprantl accepted this revision.Dec 10 2020, 8:49 AM
aprantl added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
807

Is there a way to assert while still getting the benefit of Clang's switch-doesnt-cover-all-enum-cases warning here?

This revision is now accepted and ready to land.Dec 10 2020, 8:49 AM
dblaikie added inline comments.Dec 10 2020, 3:06 PM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
807

Roughly speaking: Not really, no.

If you have a fully covered switch, you'll get a warning if you have a default (-Wcovered-switch-default). Generally unless you're reading from user input, you shouldn't worry too much about having invalid enum values - at least most of LLVM doesn't try to have defensive asserts for these sort of spoiled enum values.

djtodoro added inline comments.Dec 11 2020, 1:20 AM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
763

MIB.addReg(0U, RegState::Debug);

aprantl added inline comments.Dec 11 2020, 6:37 PM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
807

Thanks for clarifying — I guess the subtext here is that we could just remove the assertion and rely on the compiler warning instead, right?

dblaikie added inline comments.Dec 11 2020, 8:44 PM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
807

Perhaps more than that - if it's a fully covered switch over an enum, you can't put a default in it without breaking the -Werror build. (this was a long-standing LLVM style nit before I implemented the -Wcovered-switch-default warning so we didn't have to identify this issue manually, but it's still in https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations ).

So, yeah: If the switch covers all the enum values, do not include a default.

This revision was landed with ongoing or failed builds.Mar 9 2021, 4:18 AM
This revision was automatically updated to reflect the committed changes.