-
-
Notifications
You must be signed in to change notification settings - Fork 808
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
fix: block mload
merging when src and dst overlap
#3635
fix: block mload
merging when src and dst overlap
#3635
Conversation
|
||
|
||
def _merge_load(argz, _LOAD, _COPY): | ||
def _merge_load(argz, _LOAD, _COPY, allow_overlap=True): |
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.
nit: think generally params should be "safe-by-default", and the "safer" option would be to have no overlap.
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.
hmm i see, my thinking was that this only affects where the source and destination address spaces are the same, and there is only one case for this, mload
/ mstore
. but i am open to changing the default.
vyper/ir/optimizer.py
Outdated
@@ -689,6 +689,11 @@ def _merge_load(argz, _LOAD, _COPY): | |||
initial_dst_offset = dst_offset | |||
initial_src_offset = src_offset | |||
idx = i | |||
|
|||
if not allow_overlap and initial_dst_offset <= initial_src_offset <= dst_offset: |
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.
do you have to do do a + 32 somewhere here?
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 think the <=
handles that, see the output of this "partial" overlap:
@external
def partial() -> uint256:
a: uint256 = 1337
b: uint256 = 0
c: uint256 = 0
d: uint256 = 0
e: uint256 = 0
c = a
d = b
e = c
return e
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #3635 +/- ##
==========================================
+ Coverage 89.01% 89.10% +0.09%
==========================================
Files 86 86
Lines 11460 11463 +3
Branches 2606 2606
==========================================
+ Hits 10201 10214 +13
+ Misses 836 830 -6
+ Partials 423 419 -4
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
lgtm
--------- Co-authored-by: Robert Chen <[email protected]>
mload
merging when src and dst overlap
What I did
fix an optimization bug introduced in #3483 when the target architecture is
cancun
. prior to this PR, certain sequences ofmload/mstore
could incorrectly be merged intomcopy
when there is overlap between the source and destination buffers.How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture