Page MenuHomePhabricator

[DebugInfo][NFCI] Refactor LiveDebugValues containers to be pass fields
AbandonedPublic

Authored by jmorse on Fri, Aug 23, 8:11 AM.

Details

Summary

LiveDebugValues currently declares a bunch of containers to hold state about the functions being processed, and then passes references to them through several layers of method calls. IMHO, it's simpler to place these containers in the pass object, where we then won't have to repeatedly pass them around, reducing the amount one has to read.

This separates the state of the analysis into fields of the pass object, and keeps "what we're operating on right now" as method arguments. IMHO, this more clearly communicates what a method is doing.

It's possible there's some style / convention to do this that I'm not aware of, in which case, never mind.

There's some slight flakeyness with the VarLocMap being cleared: calling "reset" on the object leads to a compile error, because UniqueVector calls std::vector::resize(0, 0), a method that does not exist in the STL. I've steered clear of dealing with this, and just overwrite VarLocIDs with a newly constructed object.

Diff Detail

Event Timeline

jmorse created this revision.Fri, Aug 23, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 23, 8:11 AM

I'm not a big fan of this approach unless there is a performance aspect that would motivate it.

The reason why I chose the "pass everything around" approach was because this makes it very obvious what the dependencies of each function are and also makes it glaringly obvious that there is no hidden state that is modified and depended on by the various steps. Currently each function explicitly declares its dependencies and expectations. I do think that the stateful variant may be easier to understand for someone who is already an expert on LiveDebugValues, but the explicit variant is definitely easier to reason about for someone who has to come in understand the pass and fix a bug. I think long-term it is better to optimize for the latter case.

jmorse abandoned this revision.Fri, Aug 23, 8:37 AM

No problemo