-
Notifications
You must be signed in to change notification settings - Fork 693
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
Add Memset+Memcpy to FutureFeatures.md #1057
Conversation
FutureFeatures.md
Outdated
|
||
* `move_memory`: Copy data from one memory region to another region, even if overlapping | ||
* `zero_memory`: Set all bytes in a memory region to zero | ||
* `set_memory`: Set all bytes in a memory region to a given byte |
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.
Id it useful to distinguish zero?
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.
Not sure, but I figured I'd include both since both were discussed.
FutureFeatures.md
Outdated
### Memset and Memcpy Operators | ||
|
||
Copying and clearing large memory regions is very common. This can be done in | ||
the MVP via `i32.load` and `i32.store`, but this is not very fast. The |
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'm not sure the speed argument is well stated: a fast mem*
is dependent on architecture, and the load/store approach requires more bytecode while forcing VMs to recognize the loop.
Overall the reasons to have mem*
aren't huge, but they seem at least worth considering.
I'd also mention that some things are expected from dev-side compilers such as LLVM. I don't expect to see small mem*
operations in WebAssembly.
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.
Done.
FutureFeatures.md
Outdated
the MVP via `i32.load` and `i32.store`, but this is not very fast. The | ||
following operators can be added to improve performance: | ||
|
||
* `move_memory`: Copy data from one memory region to another region, even if overlapping |
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.
"even if" doesn't state what happens if it is overlapping.
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.
Done.
Would there be two variants of these: one which takes the size as an immediate, and another from the stack? |
We haven't done that sort of thing with any other operators, so it feels a bit out of place to start doing it now. I'm guessing this wouldn't be a very common opcode anyway, so the byte savings may not make much of a difference. |
FutureFeatures.md
Outdated
to recognize the loops as well. The following operators can be added to improve | ||
performance: | ||
|
||
* `move_memory`: Copy data from one memory region to another region, copying backward if the regions 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.
Instead of saying "copying backward", I think we should say what memmove() docs say which is that it works as-if a temporary buffer was used. (If there is an overlap and dst > src, that does entail copying backward, but if src > dst, then you want to copy forward.)
@RyanLamansky We're expecting compilers to have optimized small/constant-sized memcpy/set (into unrolled loads/stores), saving the builtin operators only for the bulk operations. |
FutureFeatures.md
Outdated
|
||
* `move_memory`: Copy data from one memory region to another region, copying backward if the regions overlap | ||
* `zero_memory`: Set all bytes in a memory region to zero | ||
* `set_memory`: Set all bytes in a memory region to a given byte |
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.
If we have set_memory
(which I think we should), then I don't think there's much reason for a specialized zero form; we're all just going to be calling memset()
anyhow.
Thanks, lgtm |
I'm not sure this is good though... how it's implemented has observable effects with shared memory. There should be a note that we need to fix this, then lgtm. |
Since these operators would be performing a bunch of non-atomic reads and writes, it doesn't seem like specifying the order in more detail would allow one to write a correct program any more than specifying as if a temporary copy was made. |
Well, this is for |
See #236 and #977. I added both
set_memory
andzero_memory
since there was discussion about this in #236.