-
Notifications
You must be signed in to change notification settings - Fork 250
Implement std::mem::memcopy procedure
#2361
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
base: next
Are you sure you want to change the base?
Conversation
Al-Kindi-0
left a comment
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!
Left some comments and questions
| @@ -5,6 +5,171 @@ use miden_utils_testing::{ | |||
| build_expected_perm, felt_slice_to_ints, | |||
| }; | |||
|
|
|||
| #[test] | |||
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.
Should we add some proptests?
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 tried to do so, but the main problem is that we can't assert the correctness of the value copied to some new location: in case of the proptest we can't come up with some formula which will represent the value here. But I'm new to the proptest dependency, so maybe I'm missing something.
stdlib/asm/mem.masm
Outdated
| #! - words handling: 15 + 16 * num_words | ||
| #! - suffix handling: 9 + 14 * num_remaining_felts | ||
| #! - in total: 66 + 14 * num_prefix_felts + 16 * num_words + 14 * num_suffix_felts | ||
| export.memcopy |
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.
Let's use the new syntax here (i.e., pub proc). And we should merge the latest next into this to make sure all tests pass against it (i.e., export should no longe be allowed).
Also, I wonder if we can simplify this procedure to not have conditional logic. For example, we could try to compute 3 tuples: (ptr1, n1), (ptr2, n2), (ptr3, n3) and then call memcopy_elements, memcopy_words, and memcopy_elements on these unconditionally.
The idea here is that if we start with a word-aligned address, the first tuple would be (read_ptr, 0) and the second tuple would be (read_ptr, n2) etc.
If it is more efficient, the tuples could be (start_ptr, end_ptr) and in that case, for word-aligned addresses, the first tuple would be just (read_ptr, read_ptr).
The main question here is if we can implement computing these tuples efficiently. We will probably need to have a helper procedure for computing "next multiple of 4" - but I think that could be done relatively efficiently.
This PR implements new
memcopyprocedure for thestd::memmodule.This procedure copies the provided number of
Felts from the read pointer to the write pointer. It usesmemcopy_wordsandmemcopy_feltsprocedures internally in order to optimize the copy.The stack transition looks like so:
Inputs:
[n, read_ptr, write_ptr]Outputs:
[]Closes: #2359.