Page MenuHomePhabricator

[DebugInfo] Process DBG_VALUE_LIST in LiveDebugVariables

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?


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


Is it not possible to do this instead?

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

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


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


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.

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.

jmorse added a subscriber: jmorse.Feb 2 2021, 7:39 AM

Looking good -- I think there are some test gaps, is there coverage for:

  • extendDef for a DBG_VALUE_VAR -- As I understand the -movements test, it's designed for range splitting?
  • DBG_VALUE_VAR with a $noreg operand isn't propagated / split
  • Reduction / replaceArg of duplicate DBG_VALUE_VAR operands

In general I think this is good -- my greatest concern is using a std::vector for DbgVariableValue, and the fact that it's cloned whenever a modification takes place. This is going to introduce a lot more allocations in the common-case, including the number-pivoting. As I understand from the history, the problem is LiveInterval having to store a huge object? Is there a plan for solving / optimising this after this patch lands? I also saw std::move is applied to DbgVariableValue in a few places, does it need an rvalue constructor for that to be effective? (I've no idea).

There's an "isReal" flag that doesn't seem to do anything and isn't documented, is this necessary?

Finally, there's the business with SpillOffset being added to expressions in insertDebugValue, but as shown in the tests, there isn't an offset in any of the spilt expressions. That'd be what I expect, because the stack layout is determined when PEI runs. Can we ditch the offset adding code and assert that it's alway zero at this stage? (Not necessary for this patch to land).


Can we make these Registers now that we're not bit-packing?


The previous method had the benefit that only those elements above the pivot point were re-written -- this will rewrite all of them. Is there an easy / obvious optimisation? I reckon we can land with this and optimise later if it turns out to be a significant burden.


Can we not use an ArrayRef for the operands reference?


Seconded; 'tis nasty


IMO, should be a SmallVector rather than a std::vector, avoids a few additional allocs.


Mega-nit: a more specific name like "KilledLocIntervals" would help readability IMO.

StephenTozer added inline comments.Feb 8 2021, 7:31 AM

Possibly - I'm not sure what the actual performance cost of setValueUnchecked is - I assume it'd be equivalent to a lookup, i.e. small. The performance cost of decrementLocNosAfterPivot shouldn't be too large, but copying the full object every time might be bad. The easiest way to handle this is probably to create a new method to check if(DbgValue.getMaxLocNo() > LocNo), or more efficiently (since this would be its only use) if(DbgValue.hasLocNoGreaterThan(LocNo)); it's still a more expensive check than getLocNo() since it requires iteration and comparison, but there's no way around that without caching, which isn't an option when we need to minimize object size.


These ones are actually indices, not Registers.


I think so as well, but I'm not sure I prefer the idea of having the same checks spread out over multiple lines with this repeated between all of them:

if (...) {
  LLVM_DEBUG(dbgs() << "Can't handle " << MI);
  return false;

Address review comments: remove unused isReal, add hasLocNoGreaterThan to prevent unnecessary modifications, replace messy vector storage of LocNo with simple C-style array, clean up messy if condition, use default-size SmallVectors where possible, renaming and formatting.

Still TODO: Add extra tests, prevent unnecessary cloning of arrays by reusing existing ones where possible.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 10 2021, 4:47 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.