feat!: Implement GC instructions #210
Conversation
ospencer
force-pushed
the
oscar/122
branch
4 times, most recently
from
b8876c4 to
92aa57f
Compare
ospencer
force-pushed
the
oscar/122
branch
2 times, most recently
from
54618d1 to
6aa04d1
Compare
spotandjake
left a comment
spotandjake
left a comment
There was a problem hiding this comment.
I just did a basic review of this it seems the TypeBuilder js api bassically ended up being the exact same minus one small difference.
I noticed some other functions we may want to implement.
My focus on this review was mostly to find out what needed updating so I didn't pay to close attention to the actual and .ml, .mli files.
| //Requires: caml_jsstring_of_string | ||
| //Requires: caml_list_to_js_array, caml_js_from_bool | ||
| function caml_binaryen_call_ref(wasm_mod, target, params, typ, is_return) { | ||
| return wasm_mod.call_ref( |
There was a problem hiding this comment.
I cannot find this function in the js bindings in v124
There was a problem hiding this comment.
I think this needs to be added to the bindings, though I don't know the process on the binaryen side as I haven't touched it before:
self['call_ref'] = function(target, operands, type, isReturned) {
return preserveStack(() =>
Module['_BinaryenCallRef'](module, target, i32sToStack(operands), operands.length, type, isReturn)
);
};| function caml_binaryen_br_on(wasm_mod, op, name, ref, typ) { | ||
| switch (op) { | ||
| case Binaryen.BrOnNull: | ||
| return wasm_mod.br_on.null(caml_jsstring_of_string(name), ref, typ); |
There was a problem hiding this comment.
Shouldn't this be br_on_null https://github.com/WebAssembly/binaryen/blob/64ba23996a10e229d46e41eb37736a55af87f79a/src/js/binaryen.js-post.js#L2480 and not take typ?
| //Provides: caml_binaryen_call_ref_get_target | ||
| //Requires: Binaryen | ||
| function caml_binaryen_call_ref_get_target(exp) { | ||
| return Binaryen.CallRef.getTarget(exp); |
There was a problem hiding this comment.
I couldn't find the Binaryen.CallRef stuff in the js bindings.
| //Requires: caml_js_from_bool | ||
| function caml_binaryen_struct_get(wasm_mod, index, ref, type, signed) { | ||
| if (caml_js_from_bool(signed)) { | ||
| return wasm_mod.struct.get_s(index, ref, type); |
There was a problem hiding this comment.
It looks like the get_s and get_u were converted to get accepting a signed bool. https://github.com/WebAssembly/binaryen/blob/64ba23996a10e229d46e41eb37736a55af87f79a/src/js/binaryen.js-post.js#L2506
There was a problem hiding this comment.
This doesn't seem to implement all of the function as of v124,
We don't seem to have the js versions of:
- We don't seem to have either the c or js bindings for array.fill
- We don't seem to have either the c or js bindings for array.new_elem
There was a problem hiding this comment.
This doesn't seem to implement all of the function as of v124,
We don't seem to have the js versions of:
- We don't seem to have either the c or js bindings for BinaryenArrayFill
- We don't seem to have either the c or js bindings for BinaryenArrayNewElem
| external is_nullable : t -> bool = "caml_binaryen_type_is_nullable" | ||
| external from_heap_type : Heap_type.t -> t = "caml_binaryen_type_from_heap_type" | ||
|
|
||
| external from_heap_type : Heap_type.t -> bool -> t |
There was a problem hiding this comment.
Just a note that this is just a fix to our existing bindings.
| mutable_ : bool; | ||
| } | ||
|
|
||
| type error = |
There was a problem hiding this comment.
Might be a good idea to link to the values here: https://github.com/WebAssembly/binaryen/blob/64ba23996a10e229d46e41eb37736a55af87f79a/src/binaryen-c.h#L3618
Just for future updates.
Might also be a good idea to document the update practice for this in #252
| var types = builder.buildAndDispose(); | ||
| return [0, caml_js_to_array(types)]; | ||
| } catch (e) { | ||
| return [1, [0, e.index, e.reason]]; |
There was a problem hiding this comment.
This looks to just throw a generic TypeError which I don't think has an index or reason.
The api looks to just ignore the error reason.
| case Binaryen.BrOnNull: | ||
| return wasm_mod.br_on.null(caml_jsstring_of_string(name), ref, typ); | ||
| case Binaryen.BrOnNonNull: | ||
| return wasm_mod.br_on.non_null(caml_jsstring_of_string(name), ref, typ); |
There was a problem hiding this comment.
Shouldn't this be br_on_non_null https://github.com/WebAssembly/binaryen/blob/6ec7b5f9c615d3b224c67ae221d6812c8f8e1a96/src/js/binaryen.js-post.js#L2484 and not take typ?
| case Binaryen.BrOnNonNull: | ||
| return wasm_mod.br_on.non_null(caml_jsstring_of_string(name), ref, typ); | ||
| case Binaryen.BrOnCast: | ||
| return wasm_mod.br_on.cast(caml_jsstring_of_string(name), ref, typ); |
There was a problem hiding this comment.
| case Binaryen.BrOnCast: | ||
| return wasm_mod.br_on.cast(caml_jsstring_of_string(name), ref, typ); | ||
| case Binaryen.BrOnCastFail: | ||
| return wasm_mod.br_on.cast_fail(caml_jsstring_of_string(name), ref, typ); |
There was a problem hiding this comment.
Shouldn't this be br_on_cast_fail
https://github.com/WebAssembly/binaryen/blob/6ec7b5f9c615d3b224c67ae221d6812c8f8e1a96/src/js/binaryen.js-post.js#L2492
spotandjake
added
the
Api
Requires some upstream Binaryen.js changes I haven't upstreamed yet.