SSA CFG populate spill set in target size heuristic#16599
SSA CFG populate spill set in target size heuristic#16599
Conversation
clonker
added
has dependencies
clonker
force-pushed
the
ssa-cfg-avoid-spilling
branch
from
ec931b8 to
96ce5c5
Compare
clonker
changed the base branch from
improve-ssa-shuffler-fix-args-region
to
develop
clonker
removed
the
has dependencies
| // (target)| v1 v2 v3 v4 v5 v6 v7 v8 v9 v10 v11 v12 v13 v14 v15 v16 v17 v18 v19 v20 | | ||
| // Status: MaxIterationsReached | ||
| // Spilled: {v1, v2, v4, v20} |
…on a dynamic spill set
- allNecessarySlotsReachableOrFinal takes spill set into account - target min counts / required in tail checks exclude spilled variables
clonker
force-pushed
the
ssa-cfg-avoid-spilling
branch
from
96ce5c5 to
4d4f87d
Compare
blishko
left a comment
blishko
left a comment
There was a problem hiding this comment.
I have some comments, but overall I think this is good.
We can also go ahead with the current version so we can have proper memory spilling working as soon as possible.
| // +------- +----------------------------------------------------------------------------------------------------------------------- | ||
| // (initial)| * | v3 v4 v5 v6 v7 v8 v9 v10 v11 v12 v13 v14 v15 v16 v17 v18 v1 | ||
| // +------- +----------------------------------------------------------------------------------------------------------------------- | ||
| // (target)| {v1} | v3 v4 v5 v6 v7 v8 v9 v10 v11 v12 v13 v14 v15 v16 v17 v18 v1 |
There was a problem hiding this comment.
It is a little bit confusing to me that here v1 is in (target), but in the previous example, v1 is not present in the (target), even though both test cases specify targetStackTailSet as {v1}.
I am not sure if this is just a visualization issue, or something about the semantics of a target.
| // Same setup as spill_arg_in_liveout but without allowSpilling: confirms the default behaviour is | ||
| // preserved and the shuffler reports StackTooDeep without retrying. |
There was a problem hiding this comment.
Can we explicitly say allowSpilling: false here?
|
|
||
| StackData const& args; | ||
| LivenessAnalysis::LivenessData const& liveOut; | ||
| SpilledVariables const* const spilledVariables; |
There was a problem hiding this comment.
Does Target really need to store the spilledVariables?
| private: | ||
| Data* m_data; | ||
| Callbacks m_callbacks; | ||
| SpilledVariables const* m_spilledVariables = nullptr; |
There was a problem hiding this comment.
I am not convinced this should live in the Stack.
It feels like an auxiliary information that should be recorded somewhere in the algorithm rather than directly in the Stack data structure.
2 participants
In a next step, the heuristic should get the currently existing spill set as initial state.
SpilledVariablestype (map ofSSACFG::ValueId→ slot) and thread it through the SSA stack abstraction viaStack::setSpilledVariables/spilledVariables().Stack::canBeFreelyGeneratedbecomes non-static and treats spilled values as freely materializable alongside literals, junk, and call return labelsStackShufflerto honor the spill setTargetconstructor accepts the spill set and excludes spilled values fromminCountand theliveOuttail countrequiredInTailreturns false for spilled values (rematerializable)findOptimalTargetSizenow iteratively retriesshuffle, adding theStackTooDeepculprit to a local spill set each round; cost function penalizes spills with+1000per spilled variableStackShufflerTestwithallowSpilling/initialSpilledSetparser keys, a pre-pass to compute the spill set, reporting of spilled values in the trace, and tail-set assertions that skip spilled IDsdepends a little bit on #16598 to deal with some emerging cycles. could theoretically also be stand-alone.