Skip to content
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

Stack layout generator stack compression oddity. #15877

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ekpyron
Copy link
Member

@ekpyron ekpyron commented Feb 17, 2025

Noticed in #15844

The thought behind the line changed here was to always minimize the propagated stack, if the required value can be dup'ed ad-hoc, but the counting was off (not even by just 1, but by 2 slots).

However, while this change improves gas cost on some tests, it triggers a stack-too-deep in an external test (this is likely due to a non-local effect of the change, which is hard to directly mitigate).

So I'd tend towards keeping the existing logic and towards fixing this with generally improved stack layout generation in the SSA codegen approach. I'm still pushing a draft PR for now to consider options.

// /* "":566:582 */
// calldataload
// /* "":591:605 */
// dup16
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the change, this dup16 will instead be transported up and turn into a dup14 plus some swapping to compensate.

@ekpyron
Copy link
Member Author

ekpyron commented Feb 18, 2025

The actual stack-too-deep failure in elementfi is actually also a bit curious. First of all, it's weird that this change triggers it - the change should actually make stacks smaller - but it will keep additional copies of slots around further up in the code, which it appears to just happen to prevent the stack-too-deep in elementfi.
But futhermore, the stack-too-deep error itself indicates that there is quite a few function return labels on stack when the stack-too-deep happens - I actually thought, if we run into stack-too-deep, we do another pass and aggressively compress the stack, which should remove all these - however, we only do that within blocks, so this may be cross-block shuffling that fails here.

In any case, I'd actually tend towards not messing with this, even though the current behaviour is odd here, but to instead focus on bringing the SSA-based code transforms up to speed, which should be much more well-behaved in general.

@cameel
Copy link
Member

cameel commented Mar 3, 2025

ElementFi test seems to be missing a memoryguard. Maybe making a few assembly blocks memory safe would still make it compile? It would be nice to get this merged rather than leave it with something that's technically a bug. And it even reduces gas usage by a tiny bit.

The particular StackTooDeep you get there also points at a specific location so even if memoryguard does not help, it should still not be that hard for the upstream devs to work around it by just tweaking the code a bit. I see that the contracts haven't been updated in 10 months but we could always try to submit a PR.

In any case, if we decide not to fix it, we should at least add a comment explaining what's happening there. I'd even refactor the condition into *offset + 1 <= 14 to make the oddity stand out more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants