Implement the `struct.*` GC instructions #9275

fitzgen · 2024-09-18T01:15:01Z

This commit implements the struct.* instructions for the GC proposal. These instructions allow allocating new structs and getting and setting their fields.

The implemented instructions are:

struct.new
struct.new_default
struct.get
struct.get_s
struct.get_u
struct.set

The struct.new* instructions are also allowed in constant expressions, but support for that is not yet implemented.

This commit implements the `struct.*` instructions for the GC proposal. These instructions allow allocating new structs and getting and setting their fields. The implemented instructions are: * `struct.new` * `struct.new_default` * `struct.get` * `struct.get_s` * `struct.get_u` * `struct.set` The `struct.new*` instructions are also allowed in constant expressions, but support for that is not yet implemented. Co-Authored-By: Trevor Elliott <[email protected]>

github-actions · 2024-09-18T02:54:20Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton

Nice progress! 👍

crates/environ/src/gc.rs

crates/cranelift/src/gc/disabled.rs

alexcrichton · 2024-09-18T02:56:45Z

crates/cranelift/src/gc.rs

@@ -48,12 +48,114 @@ pub fn gc_ref_table_fill_builtin(
    imp::gc_ref_table_fill_builtin(ty, func_env, func)
 }

+/// Translate a `struct.new` instruction.
+pub fn translate_struct_new(


If you want I think it'd be fine to pub use the inner module up to here. That loses the "type check" that it's exactly the same signature with/without gc but I think that's ok given all the CI checks we have.

alexcrichton · 2024-09-18T02:58:44Z

crates/cranelift/src/func_environ.rs

+        }
+    }
+
+    fn translate_struct_new(


Another idea, if you're interested to cut down on the duplication here, change the spec trait to be like the component compiler where it has a method to return fn gc_compiler(&mut self) -> Option<&mut dyn GcCompiler>; (in the cranelift-wasm crate) where the GcCompiler trait has all these struct translation methods. That way when gc is disabled you don't even need to impl GcCompiler, it just returns None, and only in the gc enabled case are there all the methods.

This is a good idea -- I'll do a follow up to cut down on the boilerplate but leave it as-is for this PR.

alexcrichton · 2024-09-18T02:59:37Z

crates/cranelift/src/func_environ.rs

+        // TODO: If we knew we have a `(ref i31)` here, instead of maybe a `(ref
+        // null i31)`, we could omit the `trapz`. But plumbing that type info
+        // from `wasmparser` and through to here is a bit funky.


Another alternative would be a cranelift pass perhaps? Optimizing away trapz shouldn't be too too hard in theory for most simple cases. (I realize egraphs won't work for this at all, but we can always write a custom pass too)

If it is a GC ref that this function allocated, or has already accessed, then we should be able to clean it up. In theory at least; in practice it might be a little tricky without introducing some new powers to the mid-end. But for an argument that we haven't accessed yet, then we can't really do anything without the type info, I don't think.

alexcrichton · 2024-09-18T03:06:40Z

crates/cranelift/src/gc/enabled.rs

+        WasmStorageType::I8 => {
+            builder
+                .ins()
+                .istore8(ir::MemFlags::trusted(), new_val, field_addr, 0);


Is ::trusted() the right default here? I would have thought that this would be "can possibly trap" flags. We can stuff in a debug trap code which doesn't cause trap table information to get emitted but it should still hinder optimizations and such in Cranelift (which I think we want at least for now?)

I think we actually don't want to hinder optimizations. These aren't visible to Wasm semantics, or at least the whole point is they shouldn't be, so if the optimizer can prove that a load is redundant or whatever then I think we should let it.

alexcrichton · 2024-09-18T03:08:55Z

crates/cranelift/src/gc/enabled.rs

+    /// read or write up to `size` bytes. Do NOT attempt accesses larger than
+    /// `size` bytes; that may lead to unchecked out-of-bounds accesses.
+    ///
+    /// This method is collector-agnostic.
    fn prepare_gc_ref_access(


This method may want to be refactored to return (ir::Value, u32) to enable optionally embedding the offset in the load/store instruction. For now it'd always return 0 but maybe a future refactoring to have to optimize this a bit in the future.

Yeah, I was also thinking it may want to take a object_size: Option<u32> argument as well to make bounds checks of repeated accesses dedupe-able by the mid-end. (Has to be an Option because we don't statically know array lengths).

But: simple, naive things first and we can clean up the code we generate in follow ups.

alexcrichton · 2024-09-18T03:12:39Z

tests/disas/gc/struct-new.wat

+;; @002a                               v15 = uextend.i64 v11
+;; @002a                               v16 = iconst.i64 16
+;; @002a                               v17 = uadd_overflow_trap v15, v16, user65535  ; v16 = 16


As a minor possible optimization (future PR) this should be pretty easy to optimize in the egraph pass to downgrade this to iadd. It's pretty obvious by construction that this can't overflow, along with many other patterns too.

Whether it's worth it I'm not sure. Optimizing bounds checks is naturally going to be much more involved than just this b/c we want to rely on guard pages exclusively in theory.

Ideally we'd have a mid-end ISLE rule something like

(rule (simplify (uadd_overflow_trap $i64 x y)) (if-let $true (add_cannot_overflow $I64 x y)) (iadd x y))

where add_cannot_overflow handles constants and (uextend ...)s and such.

But because uadd_overflow_trap has side effects, we can't write such a rule in the mid-end today.

crates/cranelift/src/gc/enabled.rs

fitzgen requested review from a team as code owners September 18, 2024 01:15

fitzgen requested review from elliottt and alexcrichton and removed request for a team and elliottt September 18, 2024 01:15

fitzgen force-pushed the gc-struct-instructions branch from 98daafd to 57ae451 Compare September 18, 2024 01:27

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:ref-types Issues related to reference types and GC in Wasmtime labels Sep 18, 2024

alexcrichton approved these changes Sep 18, 2024

View reviewed changes

fitzgen added 2 commits September 18, 2024 09:32

Fill in TODO doc comment

865b604

Dedupe gc-disabled messages

8f1b937

fitzgen enabled auto-merge September 18, 2024 16:42

Fix warnings in no-gc builds

c154058

fitzgen added this pull request to the merge queue Sep 18, 2024

Merged via the queue into bytecodealliance:main with commit 37ed724 Sep 18, 2024
39 checks passed

fitzgen deleted the gc-struct-instructions branch September 18, 2024 17:27

fitzgen mentioned this pull request Sep 18, 2024

Make the GC compiler interface have less boilerplate #9278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the `struct.*` GC instructions #9275

Implement the `struct.*` GC instructions #9275

fitzgen commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

alexcrichton left a comment

alexcrichton Sep 18, 2024

alexcrichton Sep 18, 2024

fitzgen Sep 18, 2024

alexcrichton Sep 18, 2024

fitzgen Sep 18, 2024

alexcrichton Sep 18, 2024

fitzgen Sep 18, 2024

alexcrichton Sep 18, 2024

fitzgen Sep 18, 2024

fitzgen Sep 18, 2024

alexcrichton Sep 18, 2024

fitzgen Sep 18, 2024

Implement the struct.* GC instructions #9275

Implement the struct.* GC instructions #9275

Conversation

fitzgen commented Sep 18, 2024

github-actions bot commented Sep 18, 2024

Subscribe to Label Action

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement the `struct.*` GC instructions #9275

Implement the `struct.*` GC instructions #9275