Page MenuHomePhabricator

[DebugInfo] LiveDebugValues: Move DBG_VALUE creation into VarLoc class

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



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
286 ↗(On Diff #219533)

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

287 ↗(On Diff #219533)

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

320 ↗(On Diff #219533)


360 ↗(On Diff #219533)

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 :-)

650 ↗(On Diff #219533)

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

780 ↗(On Diff #219533)

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.

262 ↗(On Diff #219533)

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

205 ↗(On Diff #219533)

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.

262 ↗(On Diff #219533)

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.

287 ↗(On Diff #219533)

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

360 ↗(On Diff #219533)

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.

780 ↗(On Diff #219533)

Sounds good

djtodoro added inline comments.Sep 12 2019, 4:38 AM
262 ↗(On Diff #219533)


304 ↗(On Diff #219738)

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!

333 ↗(On Diff #219899)

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.Fri, Oct 4, 3:34 AM
jmorse added inline comments.
333 ↗(On Diff #219899)

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.