Page MenuHomePhabricator

[DebugInfo] LiveDebugValues: Move DBG_VALUE creation into VarLoc class
ClosedPublic

Authored by jmorse on Sep 10 2019, 7:17 AM.

Details

Summary

In D67393 I've described a problem with LiveDebugValues having a feedback effect, which necessitates being able to delete location transfers that are later discovered to be invalid. This patch is an almost-entirely-NFC cleanup (see below for the functional difference) that aims to ease the deletion of transfers in LiveDebugValues.

The main theme here is moving the creation of DBG_VALUE instructions from two different functions into one method in the VarLoc class. A VarLoc and a DBG_VALUE are then effectively equivalent (the DBG_VALUE just describes the contents of a VarLoc), and in a later patch we can delete transfers by VarLoc number, without having to manipulate any DBG_VALUE insts. Reducing duplication has its own virtues too.

In slightly more detail: I've added a couple of factory functions (CreateCopyLoc and CreateEntryLoc) to encapsulate some slightly fiddly operations ("Use this variable location, but put it in this machine register"). BuildDbgValue implements creating a DBG_VALUE from whatever location a VarLoc describes, and condenses the code in insertTransferDebugPair and flushPendingLocs. A TransferDebugPair struct changes from being a {insert_point, DBG_VALUE} pair to being a {insert_point, VarLocID} pair. Creating a transfer now just means creating a VarLoc and recording the number. Transfer DBG_VALUEs are all created at the end of location propagation via calling BuildDbgValue.

There are a couple of subtle artefacts: previously VarLocs could effectively build long chains where the MI field referred to the previous transfer DBG_VALUE, eventually rooted in an original source-level assignment DBG_VALUE. Now, however, VarLocs can only ever refer to the originating source-level assignment DBG_VALUE, which I think is slightly neater. Plus, previously a VarLoc was effectively a proxy for a DBG_VALUE inst, to make it easier to identify, but because we use VarLoc as the source for creating DBG_VALUE insts, it now holds real meaningful state (TM). For example, the Expr field is the only place an entry-value expression is stored until a DBG_VALUE is created from it.

I'm not familiar with how entry-value information is recorded; as they appear to be register locations with additional expression information, I've recorded them as such.

This patch was going to be 100% NFC, but as it happens it fixes one small problem. Previously if an indirect DBG_VALUE was spilt, when the spill was restored it would be made a direct register reference, losing information along the way. Now, because VarLocs always point their MI field at the original source-level assignment DBG_VALUE, this information doesn't get lost. I've added a test function that records this. (Curiously, we can easily get indirect DBG_VALUEs before regalloc, from dbg.declare's of arguments).

Diff Detail

Event Timeline

jmorse created this revision.Sep 10 2019, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 7:17 AM
aprantl added inline comments.Sep 10 2019, 8:50 AM
lib/CodeGen/LiveDebugValues.cpp
277–316

///

306

nit: here we have enough horizontal space to actually write DILocalVariable and DIExpression, which may make this slightly easier to read.

307

this seems to be used only in one switch case, perhaps sink it?

356

It would be nice if this also printed the kind that we constructed it as (CopyLoc, ...) maybe using an asserts-only kind field? Maybe this also isn't necessary :-)

646

we should probably pass Out into dump and give a dbgs() default argument?

778–784

should we have a self-documenting static constructor function for this case as well?

I think that the problem described within the D67393 imposes a solution with introducing something like DBG_LOC, as @aprantl already suggested.

lib/CodeGen/LiveDebugValues.cpp
282

Is this redundant, since we already set the Kind within the VarLoc's constructor?

test/DebugInfo/MIR/X86/live-debug-values-restore.mir
205

nit: Please make this in order.

jmorse updated this revision to Diff 219738.Sep 11 2019, 9:52 AM
jmorse marked 11 inline comments as done.

Address feedback, twiddle various things.

lib/CodeGen/LiveDebugValues.cpp
282

This is deliberately building a VarLoc from the register location in MI, then modifying it to be an entry value.

This could be done differently, but I thought a function that "turns this existing DBG_VALUE into an entry-value VarLoc" would be simpler than building a whole VarLoc from scratch. I'll update the comment to indicate that's what I intended.

307

It's passed to BuildMI in each switch case (I've moved EntryValueKind to not fall-through with the latest revision).

356

Ah well, determining where a VarLoc originates from is the topic of a future patch, best leave it til then when it'll be easier to implement.

778–784

Sounds good

djtodoro added inline comments.Sep 12 2019, 4:38 AM
lib/CodeGen/LiveDebugValues.cpp
282

Thanks.

315

clang-format please

jmorse updated this revision to Diff 219899.Sep 12 2019, 6:26 AM
jmorse marked an inline comment as done.

clang-format patch

vsk accepted this revision.Sep 20 2019, 1:22 PM

Really nice!

lib/CodeGen/LiveDebugValues.cpp
333

Is the LLVM_ENABLE_DUMP guard still needed?

This revision is now accepted and ready to land.Sep 20 2019, 1:22 PM
jmorse marked an inline comment as done.Oct 4 2019, 3:34 AM
jmorse added inline comments.
lib/CodeGen/LiveDebugValues.cpp
333

My understanding is that it exists to build-in dump methods when specified on a release build -- it's not necessary, but helpful if one is debugging a release, I think.

This revision was automatically updated to reflect the committed changes.