Skip to content

Yul codegen for immutables. #8583

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

Merged
merged 4 commits into from
May 4, 2020
Merged

Yul codegen for immutables. #8583

merged 4 commits into from
May 4, 2020

Conversation

chriseth
Copy link
Contributor

@chriseth chriseth commented Apr 2, 2020

Part of #8551

@chriseth chriseth requested a review from ekpyron April 7, 2020 15:25
@chriseth chriseth force-pushed the yulForImmutables branch 2 times, most recently from 99e7b93 to 120dd77 Compare April 27, 2020 16:58
@chriseth
Copy link
Contributor Author

Still needs tests to be activated.

@chriseth chriseth marked this pull request as ready for review April 27, 2020 16:59
@chriseth chriseth force-pushed the yulForImmutables branch 2 times, most recently from d21b9fd to 3fba8a4 Compare April 27, 2020 19:23
@chriseth
Copy link
Contributor Author

Should be complete now.

Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I don't feel confident enough about this code to say that there isn't anything missing (hence no accept) but I carefully read through it all and did not notice anything that would be wrong in an obvious way. Just one stylistic remark about resetContext().

@chriseth chriseth force-pushed the yulForImmutables branch 2 times, most recently from 94d126c to 45c99aa Compare April 28, 2020 16:49
ekpyron
ekpyron previously approved these changes May 4, 2020
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Looks good, but some comments and remarks.
I'd be fine with merging as is, though, so I'm already approving.
Also: what's the plan for this for wasm? Actually not sure at the moment what state the wasm codegen is in in general.

codecopy(0, dataoffset("<object>"), datasize("<object>"))

<#storeImmutables>
setimmutable("<immutableName>", <var>)
Copy link
Member

Choose a reason for hiding this comment

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

I'm still wondering whether this should at some point have a much more general signature, like

setimmutable(0 /* memory offset of code to be modified */, "<object>", "<immutableName>", <var>)

Then immutable references would be an implicit property of yul objects and the semantics would be clearer even if e.g. there's no unique runtime object or a deploy routine that doesn't write code to zero in memory (although I'd not be sure about details, like if the memory offset should be a literal, etc.)...
But then again we might come up with an even better way in the future and I guess it's fine for now (especially since the assembly wouldn't support the extended signature right away).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Referencing the object sounds like a good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could come up with a more sophisticated codecopy function that performs the code copy and the immutable replacement at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

I'll create an issue for it.

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.

4 participants