Stream: git-wasmtime

Topic: wasmtime / PR #7352 PCC: support x86-64.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 24 2023 at 21:36):

cfallin requested abrown for a review on PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 24 2023 at 21:36):

cfallin opened PR #7352 from cfallin:pcc-x86 to bytecodealliance:main:

This PR extends the proof-carrying-code infrastructure to support x86-64 as well as aarch64. In the process, many of the mechanisms had to be made a little more general.

One important change is that the PCC leaves more "breadcrumbs" on the frontend now, avoiding the need for magic handling of facts on constant values, etc., in the backend. For the first time a lowering rule also gains the ability to add a fact to a vreg to preserve the chain as well.

With these changes, we can validate compilation of SpiderMonkey.wasm with Wasm static memories on x86-64 and aarch64:

cfallin@fastly2:~/work/wasmtime% target/release/wasmtime compile -C pcc=yes --target x86_64 ../wasm-tests/spidermonkey.wasm
cfallin@fastly2:~/work/wasmtime% target/release/wasmtime compile -C pcc=yes --target aarch64 ../wasm-tests/spidermonkey.wasm
cfallin@fastly2:~/work/wasmtime%

view this post on Zulip Wasmtime GitHub notifications bot (Oct 24 2023 at 21:36):

cfallin requested wasmtime-compiler-reviewers for a review on PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 24 2023 at 21:36):

cfallin requested fitzgen for a review on PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 24 2023 at 21:53):

cfallin updated PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen submitted PR review:

Nice!!

A few comments below.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen submitted PR review:

Nice!!

A few comments below.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

Ditto.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

Ditto.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

Will all these traces in this file continue to be useful? As written, I don't think they really have enough context to be helpful, but maybe they would with more context in the message? Or maybe it would be better to just remove them?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

Right now an XmmMem provides a strong guarantee because the field is private and all constructors do checks. We can't make these inner fields pub because that would allow someone to mutate, for example, an XmmMem such that it wraps a non-XMM register and that breaks the type's invariants.

Same for the other register class newtypes.

There should be all the various conversions available like From, TryFrom, and Into but if any aren't present then we can add them. I think we might still need AsRef implementations.

But we really shouldn't add unchecked constructors or anything that is the equivalent (like making the inner fields pub or having an AsMut implementation)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

It seems like the trace here should be the Display/Debug of the error returned, and the error should contain the instruction or instruction index.

That is a bit larger of a refactor, and can happen in a follow up PR, but we should at least in the meantime make this a log::error! rather than a crate::trace!.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:39):

fitzgen created PR review comment:

I think this would be a little bit more general and less ad-hoc or at least more aesthetically pleasing to me if we separated fact attachment from fact construction:

        (let ((fact Fact (make_range_fact 64 0 0xffff_ffff)))
          (with_fact src fact)))

But feel free to take or leave this suggestion.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:44):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:44):

cfallin created PR review comment:

Hmm, yeah, in principle I agree, but I couldn't find a way to make the pattern-matches see through these. Without the ability to pattern-match Inst::Inst { src: GprMem(RegMem::Mem { ... }), .. } vs. Inst::Inst { src: GprMem(RegMem::Reg { ... }), .. }, the code in isa::x64::pcc either becomes nightmarish or just impossible. I think we need to find a way to preserve that somehow...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:48):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:48):

cfallin created PR review comment:

The right long-term answer -- both typesafe and compatible with pattern-matching -- would be to directly have enum GprMem { Gpr { ... }, Mem { ... } }; that way the type still names the invariant, but we also can pattern-match on the interior. I don't want to couple that refactor to this work though, and unfortunately I don't see any trait impls that will let pattern-matching work otherwise; I would love to be wrong though if you know a way?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:55):

fitzgen created PR review comment:

Instead of only matching when the {Gpr,Xmm}Mem wraps a RegMem::Mem, we can always match and then call operand.as_ref() or operand.into() to get the underlying RegMem, and finally pass that into a helper that early exits when given a RegMem::Reg. Essentially pushing the ::Reg vs ::Mem filtering down one layer.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 17:55):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:02):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:02):

cfallin created PR review comment:

Right, I get that this is a workaround, but I'm saying that the code becomes nightmarish: we really do need the ability to match on the Inst overall with specific kinds of operands...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:24):

cfallin created PR review comment:

(Spoke offline with fitzgen a bit; I'm going to try to refactor in this direction anyway and see how bad it really is. Long-term it would still be nice to get to separate enums to allow native deep sub-pattern-matching!)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:24):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 01:44):

cfallin updated PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 01:45):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 01:45):

cfallin created PR review comment:

Added one in the toplevel driver loop, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 01:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 01:46):

cfallin created PR review comment:

For now I'm going to keep this just for expediency / YAGNI but let's refactor as needed when we come to other sorts of fact propagation!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 17:12):

cfallin updated PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:10):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:10):

fitzgen has enabled auto merge for PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:10):

fitzgen has disabled auto merge for PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 18:43):

cfallin updated PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 19:17):

cfallin has enabled auto merge for PR #7352.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 26 2023 at 20:11):

cfallin merged PR #7352.

Last updated: Dec 23 2024 at 12:05 UTC