-
Notifications
You must be signed in to change notification settings - Fork 6.1k
SSA CFG populate spill set in target size heuristic #16599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
e9f2108
11ca7e2
2c9d8b2
4d4f87d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,10 +39,16 @@ namespace detail | |
| /// provide a lower bound for the slot distribution. | ||
| struct Target | ||
| { | ||
| Target(StackData const& _args, LivenessAnalysis::LivenessData const& _liveOut, std::size_t _targetSize); | ||
| Target( | ||
| StackData const& _args, | ||
| LivenessAnalysis::LivenessData const& _liveOut, | ||
| std::size_t _targetSize, | ||
| SpilledVariables const* _spilledVariables = nullptr | ||
| ); | ||
|
|
||
| StackData const& args; | ||
| LivenessAnalysis::LivenessData const& liveOut; | ||
| SpilledVariables const* const spilledVariables; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does |
||
| std::size_t const size; | ||
| std::size_t const tailSize; | ||
| boost::container::flat_map<StackSlot, size_t> minCount; | ||
|
|
@@ -158,8 +164,11 @@ class StackShuffler | |
| std::size_t _targetStackSize | ||
| ) | ||
| { | ||
| detail::Target const target(_args, _liveOut, _targetStackSize); | ||
| yulAssert(_liveOut.size() <= target.size, "not enough tail space"); | ||
| detail::Target const target(_args, _liveOut, _targetStackSize, _stack.spilledVariables()); | ||
| // If the caller has wired up a spill set, the shuffler can reduce the effective liveOut | ||
| // size by spilling; otherwise the liveOut must fit into the target up front. | ||
| if (!_stack.spilledVariables()) | ||
| yulAssert(_liveOut.size() <= target.size, "not enough tail space"); | ||
| { | ||
| // check that all required values are on stack | ||
| detail::State const state(_stack.data(), target, ReachableStackDepth); | ||
|
|
@@ -272,6 +281,21 @@ class StackShuffler | |
| if (shrinkStack(_stack, _state)) | ||
| return {StackShufflerResult::Status::Continue}; | ||
|
|
||
| // if we couldn't shrink the stack we surface this failed state as stack too deep | ||
| for (StackOffset const offset: _state.stackRange() | ranges::views::reverse) | ||
| { | ||
| Slot const& candidate = _stack[offset]; | ||
| if ( | ||
| candidate.isValueID() && | ||
| !candidate.isLiteralValueID() && | ||
| ( | ||
| !_stack.spilledVariables() || | ||
| !_stack.spilledVariables()->contains(candidate.valueID()) | ||
| ) | ||
| ) | ||
| return {StackShufflerResult::Status::StackTooDeep, candidate}; | ||
| } | ||
|
|
||
| yulAssert(false, "reached final and forbidden state"); | ||
| } | ||
|
|
||
|
|
@@ -810,7 +834,7 @@ class StackShuffler | |
| } | ||
| else | ||
| { | ||
| if (_stack.isBeyondSwapRange(*depth)) | ||
| if (_stack.isBeyondSwapRange(*depth) && !_stack.canBeFreelyGenerated(_state.targetArg(offset))) | ||
| return _stack.depthToOffset(*depth); | ||
| } | ||
| } | ||
|
|
@@ -828,7 +852,7 @@ class StackShuffler | |
| std::optional<StackDepth> depth = _stack.findSlotDepth(slotAtOffset); | ||
| // it must exist | ||
| yulAssert(depth); | ||
| if (!_stack.dupReachable(*depth)) | ||
| if (!_stack.dupReachable(*depth) && !_stack.canBeFreelyGenerated(slotAtOffset)) | ||
| return _stack.depthToOffset(*depth); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Seeds the spill set with v1 from the start. Even though v1 is in the tail set and not on the stack, | ||
| // the shuffler treats it as freely generatable and finishes admissible without retries. | ||
| allowSpilling: true | ||
| initialSpilledSet: {v1} | ||
| initial: [v3, v4, v5] | ||
| targetStackTop: [v3, v4, v5] | ||
| targetStackTailSet: {v1} | ||
| targetStackSize: 3 | ||
| // ---- | ||
| // | 0 1 2 | ||
| // +--------------------- | ||
| // (initial)| v3 v4 v5 | ||
| // +--------------------- | ||
| // (target)| v3 v4 v5 | ||
| // Status: Admissible | ||
| // Spilled: {v1} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // v1 is needed both as an arg AND in the liveOut tail set, but appears only once on the stack at the top. | ||
| // The deep tail slots (depth 17 = offset 0) are beyond swap range, so v1 cannot be duplicated into the tail. | ||
| // With allowSpilling, the shuffler reports StackTooDeep with culprit v1, the test driver adds v1 to the spill | ||
| // set and retries; v1 is now freely generatable so the second iteration succeeds. | ||
| allowSpilling: true | ||
| initial: [JUNK, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18, v1] | ||
| targetStackTop: [v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18, v1] | ||
| targetStackTailSet: {v1} | ||
| targetStackSize: 18 | ||
| // ---- | ||
| // | 0 | 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 | ||
| // +------- +----------------------------------------------------------------------------------------------------------------------- | ||
| // (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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is a little bit confusing to me that here I am not sure if this is just a visualization issue, or something about the semantics of a target. |
||
| // Status: Admissible | ||
| // Spilled: {v1} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // Same setup as spill_arg_in_liveout but without allowSpilling: confirms the default behaviour is | ||
| // preserved and the shuffler reports StackTooDeep without retrying. | ||
|
Comment on lines
+1
to
+2
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we explicitly say |
||
| initial: [JUNK, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18, v1] | ||
| targetStackTop: [v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16, v17, v18, v1] | ||
| targetStackTailSet: {v1} | ||
| targetStackSize: 18 | ||
| // ---- | ||
| // | 0 | 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 | ||
| // +------- +----------------------------------------------------------------------------------------------------------------------- | ||
| // (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 | ||
| // Status: StackTooDeep (culprit: v1) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Every target arg is also in the tail set, but the tail region has zero room (tail size = 0). | ||
| // Each fix-tail iteration reports the deepest still-needed value as StackTooDeep; with allowSpilling | ||
| // the driver spills it and retries. After every value has been spilled, the stack is admissible | ||
| // because the spilled values can be regenerated freely. | ||
| allowSpilling: true | ||
| initial: [v1, v2, v3, v4, v5, v6, v7, v8] | ||
| targetStackTop: [v1, v2, v3, v4, v5, v6, v7, v8] | ||
| targetStackTailSet: {v1, v2, v3, v4, v5, v6, v7, v8} | ||
| targetStackSize: 8 | ||
| // ---- | ||
| // | 0 1 2 3 4 5 6 7 | ||
| // +-------------------------------------------------------- | ||
| // (initial)| v1 v2 v3 v4 v5 v6 v7 v8 | ||
| // +-------------------------------------------------------- | ||
| // (target)| v1 v2 v3 v4 v5 v6 v7 v8 | ||
| // Status: Admissible | ||
| // Spilled: {v1, v2, v3, v4, v5, v6, v7, v8} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Stackdata structure.