Skip to content

Improve ssa shuffler fix args region#16598

Draft
clonker wants to merge 3 commits intodevelopfrom
improve-ssa-shuffler-fix-args-region
Draft

Improve ssa shuffler fix args region#16598
clonker wants to merge 3 commits intodevelopfrom
improve-ssa-shuffler-fix-args-region

Conversation

@clonker
Copy link
Copy Markdown
Member

@clonker clonker commented Apr 15, 2026

  • introduces argsRange on target helper, inspired from SSA: Fix cycle in stack shuffler #16595 (comment)
  • reachability-gated pushes/dups in fixArgsSlot:
    • previously, we'd sometimes push/dup slots that then immediately had to be popped again
    • add
      • canReachSomeUnfilledTarget(arg) checking if there is a swap-reachable slot wanting arg, and
      • prefixReachable checking that, after growing the stack, every misaligned prefix offset is still reachable
    • with these two utils, skip pushes/dups that would leave something stranded
  • guarded fast path in fixArgsSlot:
    • guard the fast path (place the arg the target wants at the new top directly) but only under a stricter pushKeepsDeepPrefixReachable gate: every misaligned prefix offset must survive the grow plus one more op. this accounts for committing the slot to its final position right away, which means that fixing a deeper slot requires duping/pushing something else first if that slot is in its final position

Copy link
Copy Markdown
Member Author

@clonker clonker Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the guards are a bit strict perhaps. i managed to get rid of the regression in this particular case but that lead to overall worse results in the external contracts. ugh :)

@clonker clonker force-pushed the improve-ssa-shuffler-fix-args-region branch from d8106bf to 41d19bb Compare April 15, 2026 12:02
Copy link
Copy Markdown
Contributor

@blishko blishko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely shows potential, but let's see if we can minimize the regressions.

// if there are no args, we should be done now
if (_state.target().args.empty())
return {StackShufflerResult::Status::Admissible};
yulAssert(_stack.size() == _state.target().size);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not true anymore?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, the fixArgsSlot couldn't refuse to grow and would, worst case, push junk. now it can refuse and ultimately yield NoAction in which case the invariant would be violated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants