-
-
Notifications
You must be signed in to change notification settings - Fork 806
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
feat[venom]: mark loads as non-volatile #4388
base: master
Are you sure you want to change the base?
feat[venom]: mark loads as non-volatile #4388
Conversation
this commit marks load instructions (`mload`, `sload`, etc) as non-volatile, allowing them to be removed in the `remove_unused_variables` pass.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4388 +/- ##
==========================================
- Coverage 91.31% 90.44% -0.88%
==========================================
Files 113 113
Lines 16065 16075 +10
Branches 2705 2709 +4
==========================================
- Hits 14670 14539 -131
- Misses 964 1078 +114
- Partials 431 458 +27 ☔ View full report in Codecov by Sentry. |
@HodanPlodky pointed out offline -- there could be an |
for idx, inst in enumerate(bb.instructions): | ||
self.instruction_index[inst] = idx | ||
if inst.opcode == "msize" and bb not in self.reads_msize: | ||
self.reads_msize[bb] = idx |
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.
Should not this store last msize
in basic block instead of first msize
. This could be the problem in case which is showed in PR charles-cooper#53
Co-authored-by: HodanPlodky <[email protected]>
Test for msize in branches
Volatile instructions were something temporary until proper effects handling was implemented. Now that the dft pass is doing proper effects analysis and makes use of that information, this elimination should be part of it? |
I think we can generalize the analysis done here but it increases the PR scope |
this commit marks load instructions (
mload
,sload
, etc) as non-volatile, allowing them to be removed in theremove_unused_variables
pass.What I did
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture