Conversation
Co-authored-by: Spotandjake <spotandjake@hotmail.com>
Co-authored-by: Spotandjake <spotandjake@hotmail.com>
Co-authored-by: Spotandjake <spotandjake@hotmail.com>
ospencer
requested review from
alex-snezhko,
marcusroberts,
peblair,
phated and
spotandjake
as code owners
spotandjake
left a comment
spotandjake
left a comment
There was a problem hiding this comment.
Final review done, I didn't find anything critical, I just had a few nits and wanted to keep track of a few issues we should open after this is merged.
I didn't put an approval as we still have the binaryen.ml resolution that needs to be switched.
| }; | ||
| }; | ||
|
|
||
| let build_basic_func_type = arity => { |
There was a problem hiding this comment.
Think we could either move the + 1 for the closure arg to the callee or rename this to something like build_basic_grain_func_type.
| // TODO(#): Use a more descriptive name once we get fixes into Binaryen | ||
| let grain_memory = "0"; | ||
|
|
||
| let page_size = 65536; |
There was a problem hiding this comment.
NOTE: Once this is merged, we should open an issue to move this to Grain_utils.Config and have this read from that along with the page_size definition in runtime/memory.gr. The two reasons for this are, we shouldn't store the same information in two places and it gets us set up to easily support the custom page sizes proposal.
| @@ -478,11 +339,21 @@ let compile_bind = | |||
| }; | |||
| }; | |||
|
|
|||
| let get_swap = (~ty as typ=Types.Unmanaged(WasmI32), wasm_mod, env, idx) => | |||
| let get_swap = (~ty as typ=Types.GrainValue, wasm_mod, env, idx) => | |||
There was a problem hiding this comment.
NOTE: Once this is merged we should probably open an issue to refactor get_swap, set_swap and tee_swap to a single swap_help that takes an action as thats the only part that differs.
Then we can just do:
let get_swap = (~ty as typ=Types.GrainValue(GrainAny), wasm_mod, env, idx) =>
swap_help(BindGet, ~ty, wasm_mod, env, idx);
let set_swap = (~ty as typ=Types.GrainValue(GrainAny), wasm_mod, env, idx) =>
swap_help(BindSet({value: value}), ~ty, wasm_mod, env, idx);
let tee_swap = (~ty as typ=Types.GrainValue(GrainAny), wasm_mod, env, idx) =>
swap_help(BindTee({value: value}), ~ty, wasm_mod, env, idx);
| load(~offset=4 * (idx_int + 2), wasm_mod, tup()), | ||
| Expression.Struct.get( | ||
| wasm_mod, | ||
| 2, |
There was a problem hiding this comment.
NOTE: We may want to open an issue after this is merged to build a little api to allow referencing struct fields by name rather than index.
This would make it easier to read through the compiler and help make the compiler more resilient to data layout changes.
| compile_imm(wasm_mod, env, List.nth(args, 0)), | ||
| array_type, | ||
| ), | ||
| compile_imm(wasm_mod, env, List.nth(args, 1)), // offset |
There was a problem hiding this comment.
~I really like how we use comments to document what arguments we're passing it would have been nice if we did this throughout. Might be something to come back to in a future pr though.~~~
Actually thinking about this just a little bit more, I think the case I would be making is that we should expose labelled arguments in Binaryen.ml and then use the labels in compcore to make it easier to tell what the arguments were working with are.
| Module.Feature.mutable_globals, | ||
| Module.Feature.reference_types, | ||
| Module.Feature.gc, | ||
| Module.Feature.bulk_memory, |
There was a problem hiding this comment.
I think we removed the part where we disabled the bulk_memory feature when bulk memory is not disabled.
Not sure if we should be doing the same thing for tail_call?
| }, | ||
| "dependencies": { | ||
| "@grain/binaryen.ml": ">= 0.34.0 < 0.35.0", | ||
| "@grain/binaryen.ml": "*", |
There was a problem hiding this comment.
NOTE: Once binaryen.ml is out with binaryen v126 we need to change this line
| "@opam/ocamlfind": "1.9.8", | ||
| "@opam/num": "1.5" | ||
| "@opam/num": "1.5", | ||
| "@grain/binaryen.ml": "grain-lang/binaryen.ml:package.json#451cb0df06f7254fffc5eec02ab31ec495d83340" |
There was a problem hiding this comment.
NOTE: Once binaryen.ml is out with binaryen v126 we need to change this line
| _F64_POWERS10_FAST_PATH | ||
| use WasmI32.{ (+), (<<), (|) } | ||
| let data = DataStructures.getBytesArrayRef(tableRef) | ||
| let lo = WasmArrayRef.getI8U(data, index + 3n) << 24n |
There was a problem hiding this comment.
NOTE: I think once this pr is merged, we should open an issue related to adding API's for getting larger integers from the int8 array, I think the api needs workshopping though.
This commit makes `WasmRef` unsafe, it also adds `WasmRef.(==)` and `WasmRef.(!=)` as the errors for unsafe types recommend using those, I figured it was better to provide them so the errors could be correct. We add two new warnings specifically for print and toString uses of a `WasmRef` as they are unlikely to be correct and we can't safely print or stringify a generic wasmref if it's not a subtype of GrainValue.
ospencer
force-pushed
the
oscar/gc-rebased
branch
from
d5d78e7 to
35e071d
Compare
2 participants
Wasm GC
At last, this PR switches Grain from full use of linear memory to Wasm GC objects! Details about the layouts of these objects can be found in
docs/contributor/data_representations.md.Highlights
Breaking changes
@disableGChas been removed.Closes #2335
Closes #2197
Closes #2123
Closes #989
Closes #841
Closes #813
Closes #703
Closes #2095
Closes #984
Closes #1330
Closes #2097