feat[venom]: add memory SSA analysis#4555
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4555 +/- ##
==========================================
- Coverage 92.70% 92.42% -0.29%
==========================================
Files 123 125 +2
Lines 17571 18017 +446
Branches 2988 3110 +122
==========================================
+ Hits 16290 16653 +363
- Misses 883 931 +48
- Partials 398 433 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| offset: int = 0 | ||
| size: int = 0 | ||
| is_volatile: bool = False |
There was a problem hiding this comment.
what does is_volatile mean? please add a comment
| tracking memory definitions and uses explicitly. | ||
| """ | ||
|
|
||
| VALID_LOCATION_TYPES = {"memory", "storage"} |
There was a problem hiding this comment.
can we use vyper/evm/address_space.py here? it already has metadata like the load opcode corresponding to the location.
There was a problem hiding this comment.
I think any unification refactoring should come after. I don't have the visibility to judge this atm.
vyper/venom/basicblock.py
Outdated
| return self == FULL_MEMORY_ACCESS | ||
| if self == EMPTY_MEMORY_ACCESS or other == EMPTY_MEMORY_ACCESS: | ||
| return False | ||
| if self.size <= 0 or other.size <= 0: |
There was a problem hiding this comment.
size < 0 means full memory, right? feels a bit weird to handle it separately from FULL_MEMORY_ACCESS. also, what do you think about having a proper sentinel instead of -1? like MemoryAccess.UNBOUNDED_ACCESS = object()
There was a problem hiding this comment.
| if self.size <= 0 or other.size <= 0: | |
| assert self.size > 0 and other.size > 0 # guaranteed by previous checks |
| self.current_def[block] = mem_def | ||
| self.inst_to_def[inst] = mem_def | ||
|
|
||
| def _insert_phi_nodes(self): |
There was a problem hiding this comment.
since this doesn't actually insert physical phi instructions, can we call it like compute_phi_nodes or similar?
There was a problem hiding this comment.
I don't know... feels like insert to me as we are inserting those into the memory graph (even though the graph is now separate from the other data structures, compared to our "instruction based" variable ssa phi)
There was a problem hiding this comment.
the relatedly named _add_phi_nodes function in MakeSSA does add physical instructions, so it's a bit confusing
vyper/vyper/venom/passes/make_ssa.py
Lines 34 to 37 in b396d12
at least add a comment saying that it's adding the nodes to the graph, not instructions to the basic block, otherwise it seems like the function body does not match the name
| return True | ||
| if other == FULL_MEMORY_ACCESS: | ||
| return self == FULL_MEMORY_ACCESS | ||
| if self == EMPTY_MEMORY_ACCESS or other == EMPTY_MEMORY_ACCESS: |
There was a problem hiding this comment.
not sure this is right. if other == EMPTY_MEMORY_ACCESS we should return true, right?
There was a problem hiding this comment.
Only if we are FULL_MEMORY_ACCESS, which is handled in the beginning.
| elif opcode == "dloadbytes": | ||
| return EMPTY_MEMORY_ACCESS | ||
| elif opcode == "dload": | ||
| return EMPTY_MEMORY_ACCESS |
There was a problem hiding this comment.
technically, dload writes and reads from location 0
There was a problem hiding this comment.
Yes I know, just not taking the special memory location into account for this.
vyper/venom/analysis/mem_alias.py
Outdated
| if loc1 == EMPTY_MEMORY_ACCESS or loc2 == EMPTY_MEMORY_ACCESS: | ||
| return False | ||
|
|
||
| if loc1.size <= 0 or loc2.size <= 0: |
There was a problem hiding this comment.
maybe assert loc1.size > 0 and loc2.size > 0
charles-cooper
left a comment
charles-cooper
left a comment
There was a problem hiding this comment.
lgtm. i didn't review some details in the clobbered/clobbering functions as carefully since i assume they will evolve with further work. @harkal please draft a commit message and then we can merge this!
charles-cooper
changed the title
3 participants
What I did
Implement the Memory SSA analysis for Venom
How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture