fitzgen requested elliottt for a review on PR #9275.
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:
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.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen requested wasmtime-core-reviewers for a review on PR #9275.
fitzgen requested wasmtime-compiler-reviewers for a review on PR #9275.
fitzgen requested alexcrichton for a review on PR #9275.
fitzgen updated PR #9275.
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:
- fitzgen: wasmtime:ref-types
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton created PR review comment:
TODO
alexcrichton submitted PR review:
Nice progress! :+1:
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.
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)
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?)
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.
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 theGcCompiler
trait has all these struct translation methods. That way when gc is disabled you don't even need to implGcCompiler
, it just returnsNone
, and only in the gc enabled case are there all the methods.
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.
alexcrichton created PR review comment:
Perhaps route these all to a shared definition of a function to avoid needing to duplicate the message?
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 forstruct.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)
fitzgen submitted PR review.
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.
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.
fitzgen submitted PR review.
fitzgen submitted PR review.
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.
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 anOption
because we don't statically know array lengths).
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
But: simple, naive things first and we can clean up the code we generate in follow ups.
fitzgen submitted PR review.
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.
fitzgen submitted PR review.
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.
fitzgen updated PR #9275.
fitzgen has enabled auto merge for PR #9275.
fitzgen updated PR #9275.
fitzgen merged PR #9275.
Last updated: Dec 23 2024 at 13:07 UTC