Page MenuHomePhabricator

[DebugInfo] Process DBG_VALUE_LIST in LiveDebugVariables
Needs ReviewPublic

Authored by StephenTozer on Jul 15 2020, 11:25 AM.



Continuing the work discussed in the RFC[0], this patch implements DBG_VALUE_LIST handling in the LiveDebugVariables pass.

This is one of the larger pass changes involved with handling DBG_VALUE_LIST, second only to LiveDebugValues, as this pass is involved quite in-depth with debug values. The majority of the changes made here are relatively simple, however, largely falling into two categories:

The largest logical changes take place in computeIntervals, extendDef, and addDefsFromCopies; when extending the def of a DBG_VALUE_LIST the live ranges of every used register must be considered, and when such a def is killed by more than one of its used registers being killed at the same time it is necessary to find valid copies of all of those registers to create a new def with.

Many of the remaining changes concern the interface of DebugVariableValue, which has been modified to support DBG_VALUE_LIST. Besides using a list of location numbers instead of just one, some of the functions have also changed; changeLocNo now requires the old location number that is being replaced, remapLocNos is used when more than one location needs to be changed at once, and decrementLocNosAfterPivot is used to decrement location numbers when a location is deleted and the array indices shift. Also, the constructor of DebugVariableValue is used to remove any duplicate locations in the location list; these are manageable in the instructions themselves, but cause problems with the kind of in-depth analysis that LiveDebugVariables does.


Diff Detail

Event Timeline

StephenTozer created this revision.Jul 15 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 11:25 AM

Remove unused MF attributes.

Hi, sorry for leaving this for so long! I think it will take me a couple of passes to fully understand it, but for now I left some inline nits/questions. Please feel free to leave the nit changes until later revisions.

I'm hoping you can clarify something about addDefsFromCopies for me. IIUC without the patch we are adding defs from copies at kill points if possible. And with the patch a def is only added on the _first_ kill. Is that right?


It looks like this class was trying to be small on purpose, and it is now relatively much larger. I don't know if it will make a significant impact or not, have you managed to run any kind of performance tests with this patch yet?


IIRC you mentioned that DBG_VALUE_LIST don't have the 'IsIndirect' field. It may be worth adding an assert here that !(WasIndirect && WasList)?


IMO it'd be nice to make these default = false and remove the default constructor.


Please can you update this comment, and the one for addDefsFromCopies?


This if looks a bit gnarly to me. I would personally prefer a couple of simpler early exit checks instead, though I'm not sure if there's anything in the style guide to mandate it.


Is it not possible to do this instead?

LLVM_DEBUG(dbgs() << "\t[" << Start << ';' << Stop << "):";

Rebased onto recent master; no functional changes from rebase.

Also changed DbgVariableValue to hold a pointer to a vector of location numbers, instead of containing a SmallVector of the same. This change was necessary to reduce the size of the DbgVariableValue class so that it can be used with an IntervalMap without error. This solution, however, is most likely not optimal in terms of efficiency. As such it is likely that this implementation will need a change; I have uploaded it as-is for two reasons: firstly to ensure that we have a working stack of patches available immediately, and secondly to allow review for the current implementation as most of the changes will not be affected by a future fix unless it involves a total rewrite of LiveDebugVariables.

I'm hoping you can clarify something about addDefsFromCopies for me. IIUC without the patch we are adding defs from copies at kill points if possible. And with the patch a def is only added on the _first_ kill. Is that right?

That's what it says on the tin, but after actually examining the code that calls addDefsFromCopies it appears that we never passed more than one kill point before this change, hence the slight rewrite. More specifically, addDefsFromCopies is only called in one place; in that place, the Kills vector is declared locally and then passed to extendDef immediately before that call, and extendDef always returns immediately after adding a single item to Kills, meaning that it's really more of an optional than a vector.


I did notice this when changing the class, and I share the concerns. Performance testing this implementation is in-progress, but I imagine that if the size is a concern then it should be sufficient to shrink the size of the array. In most cases we only actually need one element, so compared to the old class size size of 12 bytes with 64-bit pointers, this would be 21 bytes - an increase, but hopefully not drastic. If this turns out to still cause a major performance hit though, then it will probably need a more drastic redesign. I'm optimistic here however, as LiveDebugVariables is not usually a performance bottleneck (compared to LiveDebugValues).


When the ostream returned by dbgs() falls out of scope, it adds a banner (with a newline) and flushes output; we only want this to happen once for the entire print statement here, so we need to assign it to a local variable so that we can use the same object between statements.

Rebased onto recent master, fixed test failures.