feat[venom]: mark loads as non-volatile#4388
feat[venom]: mark loads as non-volatile#4388charles-cooper merged 53 commits intovyperlang:masterfrom
Conversation
this commit marks load instructions (`mload`, `sload`, etc) as non-volatile, allowing them to be removed in the `remove_unused_variables` pass.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4388 +/- ##
=======================================
Coverage 92.49% 92.50%
=======================================
Files 127 128 +1
Lines 18371 18432 +61
Branches 3180 3191 +11
=======================================
+ Hits 16992 17050 +58
- Misses 939 941 +2
- Partials 440 441 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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.
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: HodanPlodky <[email protected]>
|
i investigated refactoring (or at least trimming down) VOLATILE_INSTRUCTIONS and replacing it with essentially checking if the instruction is a terminator instruction or has write effects. however, it was not that clean since we still need to special-case MSIZE. another approach which simplifies the code here would be to have a special volatile instruction like |
vyper/codegen/module.py
Outdated
| # see py-evm extend_memory: after_size = ceil32(start_position + size) | ||
| if immutables_len > 0: | ||
| deploy_code.append(["iload", max(0, immutables_len - 32)]) | ||
| deploy_code.append(["itouch", max(0, immutables_len - 32)]) |
There was a problem hiding this comment.
let's update the comment above to reflect the new instruction
vyper/evm/opcodes.py
Outdated
| "DEBUGGER": (None, 0, 0, 0), | ||
| "ILOAD": (None, 1, 1, 6), | ||
| "ISTORE": (None, 2, 0, 6), | ||
| "ITOUCH": (None, 1, 0, 6), |
There was a problem hiding this comment.
shouldn't the cost be higher than iload given it also pops the value from the stack?
There was a problem hiding this comment.
(btw i think issues with gas estimates are at best a UX issue since we don't use them for anything besides providing gas estimates in the ABI and IR printouts)
charles-cooper
force-pushed
the
feat/improve-volatile
branch
from
8f49bf7 to
476507c
Compare
Co-authored-by: Harry Kalogirou <[email protected]>
5 participants
What I did
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture