-
Notifications
You must be signed in to change notification settings - Fork 6.3k
SSACFG backend: add stack abstraction #16273
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
Conversation
d78bc8b to
fb5d814
Compare
blishko
left a comment
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 approving this, but it would be good if someone else takes a look as well.
1499e00 to
edcef85
Compare
libyul/backends/evm/ssa/Stack.h
Outdated
| yulAssert(dupReachable(_offset), "Stack too deep"); | ||
| m_data->push_back((*m_data)[_offset.value]); | ||
| if constexpr (!std::is_same_v<Callbacks, NoOpStackManipulationCallbacks>) | ||
| m_callbacks.dup(_offset.value + 1); |
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 having some doubts here.
Why are we calling it with offset + 1?
Can you clarify if the callbacks expect Offset or Depth in dup and swap?
Maybe the strong type can be used already in the concept, to avoid any confusion?
Maybe your originally version was better? 😅
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.
The callback expects it as EVM instruction. So DUP1 is a dup(Depth{0}) is a dup(Offset{size() - 1}). Which means I was too tired apparently to implement the change properly. ^^ Let me fix that.
blishko
left a comment
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.
LGMT!
No description provided.