feat[venom]: better mem elision#4851
Conversation
HodanPlodky
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c13bc52c04
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if read_loc.alloca.alloca_size != read_loc.size: | ||
| return |
There was a problem hiding this comment.
The translation logic only checks that the source allocation is fully copied (read_loc.alloca.alloca_size == read_loc.size) before mapping write_loc.alloca to the source. If the destination allocation is larger than the copied size, later reads from the uncopied portion of destination are still rewritten to the source by _try_update_from_translates, which can return incorrect data. A full-allocation translation should only be created when the destination is fully covered as well (or guarded by range checks on rewritten reads).
Useful? React with 👍 / 👎.
| if Effects.MEMORY in inst.get_read_effects(): | ||
| self._try_update_from_translates(inst) |
There was a problem hiding this comment.
This pass rewrites every memory-read instruction using total_translation before opcode dispatch, but the _volatile_memory(inst) path in the same loop does not clear total_translation. As a result, a volatile writer such as invoke/call can clobber translated allocations, and a later read in the same block may still be redirected through stale mapping, producing wrong values. Translation state should be invalidated on volatile memory writes just like copy state.
Useful? React with 👍 / 👎.
📊 Bytecode Size Changes (venom)
Full bytecode sizes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4851 +/- ##
==========================================
- Coverage 92.93% 92.91% -0.03%
==========================================
Files 157 157
Lines 21961 22090 +129
Branches 3931 3968 +37
==========================================
+ Hits 20410 20524 +114
- Misses 1026 1033 +7
- Partials 525 533 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from vyper.venom.basicblock import IRBasicBlock, IRInstruction, IRVariable, IRLiteral | ||
| from vyper.venom.effects import Effects, to_addr_space | ||
| from vyper.venom.memory_location import MemoryLocation | ||
| from vyper.venom.memory_location import MemoryLocation, Allocation, get_memory_read_op, update_read_location, update_write_location |
Check notice
Code scanning / CodeQL
Unused import Note
1 participant
What I did
Improved memory copy elision to achieve detection of full copies between different allocations. This can at the end cause total elimination of that allocation if it is only used temporarily.
Example:
before
after
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture