Skip to content
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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 3, 2023
Merged

fix: block mload merging when src and dst overlap #3635

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2191fc3
fix: block mload merging when src and dst overlap
charles-cooper Oct 1, 2023
f01eb07
use continue instead of break
charles-cooper Oct 1, 2023
00150f7
fix the condition
charles-cooper Oct 1, 2023
a126502
fix the condition again
charles-cooper Oct 2, 2023
5580e98
add test for mload merge overlap
charles-cooper Oct 2, 2023
066f212
fix the condition to allow full overlap to become mcopy
charles-cooper Oct 2, 2023
668ba84
add more unit tests for merge_mload
charles-cooper Oct 2, 2023
d739d09
add more mload merge tests
charles-cooper Oct 2, 2023
a0ef351
add more mload merging tests
charles-cooper Oct 3, 2023
cb71178
fix lint
charles-cooper Oct 3, 2023
d996a1e
clarify a couple test cases
charles-cooper Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions vyper/ir/optimizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,10 +662,10 @@ def _rewrite_mstore_dload(argz):
def _merge_mload(argz):
if not version_check(begin="cancun"):
return False
return _merge_load(argz, "mload", "mcopy")
return _merge_load(argz, "mload", "mcopy", allow_overlap=False)


def _merge_load(argz, _LOAD, _COPY):
def _merge_load(argz, _LOAD, _COPY, allow_overlap=True):
Copy link
Contributor

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.

Copy link
Member Author

@charles-cooper charles-cooper Oct 1, 2023 •
edited
Loading

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.

# look for sequential operations copying from X to Y
# and merge them into a single copy operation
changed = False
Expand All @@ -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:
Copy link
Contributor

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?

Copy link
Member Author

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

# dst and src overlap, block the optimization
break

if (
initial_dst_offset + total_length == dst_offset
and initial_src_offset + total_length == src_offset
Expand Down
Loading