-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat: implement 0x37 - CALLDATACOPY Opcode #197
Conversation
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.
Hey! Welcome, great first pr 🦾
Can we check if we can use slice instead of looping and copying?
|
||
// For out of bound bytes, 0s will be copied. | ||
if (i + offset >= call_data.len()) { | ||
assert(*results[i] == 0, 'wrong data value'); |
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.
ah it's unlucky we can't assert equality for memory and felt dict, we should probably later create test utils for asserting equality @enitrat
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
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, will merge after thinking a bit about the type conversion thing :)
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
…ation.cairo Co-authored-by: Mathieu <[email protected]>
…ation.cairo Co-authored-by: Mathieu <[email protected]>
Snapshot Comparison Report: No changes in gas consumption. |
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
out_of_bounds_bytes.append(0); | ||
}; | ||
|
||
self.memory.store_n(out_of_bounds_bytes.span(), dest_offset + slice_size); |
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.
it looks non optimal to do two times memory.store_n
because this function is quite expensive.
I think that it's cheaper to first pad data_to_copy
, then to store_n
only once, @enitrat
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.
@ClementWalter data_to_copy
is a span and can't be padded, the solution would be to clone it into an array and then pad this array, but then I'm not sure it would actually be more effective. I will check and report back
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
if (copied.len() < size) {
let n_zeroes = size - copied.len();
self.memory.store_n(copied.pad_end_zeroes(n_zeroes).span(), dest_offset);
} else {
self.memory.store_n(copied, dest_offset);
} looks to be more optimized - I will re-verify once the requested changes have been applied |
Given that this implies cloning an entire memory segment just to pad zeroes, I think there might be a point at which it becomes more expensive to do a clone than it is to call twice store_n. Will research this later in #278 |
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
Tests will be fixed after merge PR #331 |
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
crates/evm/src/tests/test_instructions/test_environment_information.cairo
Outdated
Show resolved
Hide resolved
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.
Looks good now ! Waiting for bugfix PR merge to finish. Sorry it was a bit long but it was the first PR you opened, now it's going to be easier 💪
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.
💯
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Resolves: #123
What is the new behavior?
Does this introduce a breaking change?