-
Notifications
You must be signed in to change notification settings - Fork 244
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 experimental memory.discard instruction #882
Conversation
efce5be
to
f81c17e
Compare
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 to me, thanks! With a new fresh proposal gating this instruction happy to merge
crates/wasmparser/src/lib.rs
Outdated
@@ -325,6 +325,7 @@ macro_rules! for_each_operator { | |||
@bulk_memory DataDrop { data_index: u32 } => visit_data_drop | |||
@bulk_memory MemoryCopy { dst_mem: u32, src_mem: u32 } => visit_memory_copy | |||
@bulk_memory MemoryFill { mem: u32 } => visit_memory_fill | |||
@bulk_memory MemoryDiscard { mem: u32 } => visit_memory_discard |
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.
The @bulk_memory
here is used as a proposal to gate access to this instruction, so this'll want to be something other than bulk_memory
since otherwise memory.discard
will be enabled by default.
In lieu of an official proposal, could this perhaps be something like @experimental
or @memory_control
? That'll require adding a field to WasmFeatures
which is ok and would be necessary for this anyway.
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'll add @memory_control
I think!
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.
Added a @memory_control
feature. I hope I set true/false correctly in the various places where it was required.
f81c17e
to
bbb4a41
Compare
All looks good to me, thanks again! I think CI is showing there's at least one more place to update |
bbb4a41
to
dc5b116
Compare
Just patching up those last couple places. Didn't realize I had to run |
dc5b116
to
3d2be3c
Compare
I stepped over myself a bit during publication but these crates are now all published on crates.io |
Adding support for a
memory.discard
instruction proposed in the memory control proposal, which I am prototyping within SpiderMonkey.@eqrion