-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Code Generation: Constant creation with NOT(0) + shifts #16729
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
Open
DanielVF
wants to merge
7
commits into
argotorg:develop
Choose a base branch
from
DanielVF:moh-eulith-constants
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
213a1bd
constant optimizer assembly tests
5da3315
constant optimizer: a semantic test
8ec3bc4
formatting and comments
98bdd2e
mask generation for yul constant optimizer
2b974c3
regenerate test output
418c1cc
Refactor constant optimization code for clarity
DanielVF 6f2918c
Update gas pricing in tests
DanielVF File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is interesting!
We could try to separate this change and see what effect we get from this small change.
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.
this depends on the computed mask variables (
onesEnd,onesStart), so separating it would require a chunk of code duplicated in both PRs.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.
What I am suggesting is to keep the computation of
onesEndandonesStart, but skip the computation ofnewRoutine. Do you expect that would still provide some benefit?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.
Yes, it would provide a tiny benefit for the rare cases like the given example (easy to see in a test case, hardly move the needle for real contracts). IMHO, not worth a separate PR.
P.S. Did you notice yul optimizer doesn't need this? It's because it's structurally better than the libevmasm version (yul compares different computations; libevasm compares a single computation with other choices, like
DATACOPY, so the single computation has to have extra conditionals).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 definitely see some differences. If yul version does something better, the evmasm version could be improved to do the same thing, no?
In any case, no need to separate this into a new PR.
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.
That's weird. The tests included in this repo's CI tell a different story. When I added the not(0) optimizations to the yul path, there were improvements. Compare
#15935 (comment)
and
#15935 (comment)
For example, "colony" went from -1.91 to -2.33, etc.
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.
This is using the not(0) constant optimizer as the baseline here, so it's comparing the not(0) constant optimizer PR alone (0 changes by definition) vs having both the not(0) constant optimizer and removing the yul constant optimizer (the spat version).
The not(0) constant optimizer is definitely a win in either case.
Uh oh!
There was an error while loading. Please reload this page.
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.
Okay, I now get it, and now I also understand why on the performance optimization side I've been seeing things that only speed up yul speed up the regular compiler too.
appendYulUtilityFunctions()is called byappendMissingFunctions(), and places yul code into contracts. Doing this runs the yul side, including yul optimizers, even on non ir-contracts.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.
@moh-eulith Here's the chart, with both against a baseline of the current development branch. This is probably what you were expecting.
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 assume "splat" is without the yul optimizer. I think the code this is benchmarking against matters. I just tried this on the my own contracts: removing yul-constant-optimizer increased the contract total size from 61,593 to 61,903 (I compile with
--via-ir --optimize --optimize-runs 2so presumably similar to yourvia-ir-low-runs).@blishko 's PR shows the same thing (#16738 (comment)) where ir-optimize-evm+yul has some regressions and some improvements.