Stream: git-wasmtime

Topic: wasmtime / PR #9275 Implement the `struct.*` GC instructions

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:15):

fitzgen requested elliottt for a review on PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:15):

fitzgen opened PR #9275 from fitzgen:gc-struct-instructions to bytecodealliance:main:

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:

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

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:15):

fitzgen requested wasmtime-core-reviewers for a review on PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:15):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:15):

fitzgen requested alexcrichton for a review on PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 01:27):

fitzgen updated PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 02:54):

github-actions[bot] commented on PR #9275:

Subscribe to Label Action

cc @fitzgen

<details>
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:

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton submitted PR review:

Nice progress! :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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?)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

Perhaps route these all to a shared definition of a function to avoid needing to duplicate the message?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 03:14):

alexcrichton created PR review comment:

One idea for a future trick: we could unmap the lowest page of memory in the GC heap so address 0 in the gc heap translates to a fault. That way we could push this trap into an annotated trap code on the MemFlags on the load below (or store for struct.set) which means we could, by construction, completely eliminate this trap as well.

(possibly all on top of a cranelift optimization pass to remove the checks entirely)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:16):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:16):

fitzgen created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:18):

fitzgen created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:18):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:20):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:20):

fitzgen created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:21):

fitzgen created PR review comment:

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).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:21):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:23):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:23):

fitzgen created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:27):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:27):

fitzgen created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:30):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:30):

fitzgen created PR review comment:

We discussed this a bit in today's cranelift meeting. In summary, this is neat but kind of conflicts with our plans of eventually using linear memories to implement the GC heap, since the pooling allocator would have to allocate a linear memory for the GC heap, then unmap the first page, give it to Wasm until the instance is done, then remap the first page before returning it to the memory pool. This is all doable of course, but the extra special-case code paths are annoying and adding more virtual memory calls in the pooling allocator is always fraught perf-wise.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:42):

fitzgen updated PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:42):

fitzgen has enabled auto merge for PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 16:59):

fitzgen updated PR #9275.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2024 at 17:27):

fitzgen merged PR #9275.

Last updated: Nov 22 2024 at 17:03 UTC