wasmtime / PR #2323 Rewrite interpreter generically · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2323 Rewrite interpreter generically

Wasmtime GitHub notifications bot (Oct 26 2020 at 04:31):

abrown opened PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Oct 26 2020 at 04:31):

abrown requested fitzgen for a review on PR #2323.

Wasmtime GitHub notifications bot (Oct 26 2020 at 18:48):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Oct 26 2020 at 22:01):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Oct 27 2020 at 19:13):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:25):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:25):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:25):

fitzgen created PR Review Comment:

You should be able to avoid the is_nan() checks and just defer to f32's PartialOrd implementation, as it does these same NaN checks.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

seems like this doc comment never got finished

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

It seems strange that we have an empty Interpreter struct and all of its methods take &mut InterpreterState as its first parameter. It seems like it would make sense to put merge them together and make all of these methods take &mut self. I see this is sort of similar to what used to exist, so I'm unclear on the motivation for the split as it currently is written.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

It seems strange that the interpreter enables all target architectures. I would expect it to be possible to enable none of them, and simply use the interpreter to execute target-independent clif.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

ditto

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

This code is really macro heavy. I understand the draw here: there is a lot of duplication going on. But it is pretty hard to understand, with all of the implicitly closed over arguments, and macro rules being kinda hard to read in general.

Would it be possible to refactor these helpers into closures? I think that would help readability a bunch. For example the arg! macro and calls like arg!(1) would become
let arg = |ctx, i| {
    let value_ref = ctx.args()[i];
    state
        .get_value(value_ref)
        .ok_or(StepError::UnknownValue(value_ref))
};

// ...

arg(inst_context, 1)?
(You may have to add some type annotations to the closures, but I think it would be an improvement nonetheless)

Also a quick sentence in a documentation comment for each helper would help readers understand what they're looking at here.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:25):

fitzgen created PR Review Comment:

Any reason in particular not to do this instead:
        f32::from_bits(self.0).is_nan()
?

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

ditto

                _ => unreachable!(),

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

I feel like this trait's overview documentation could use some love:

Explain why the interpreter is generic over a trait, and what this enables

Give an overview of the different things that the trait abstracts -- frames, values, flags, heap memory, and stack memory -- and what they are and how they're used.

A sketch of an example of implementing this trait (def not full impl, just informative sketch) for simple abstract interpretation (maybe signedness of values only and ignoring memory/flags/etc?) and a link to concrete implementations that exist.

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

because other opcodes should actually be impossible:
                _ => unreachable!(),

Wasmtime GitHub notifications bot (Oct 30 2020 at 18:26):

fitzgen created PR Review Comment:

Out of curiosity, how do your static analyses handle control flow joins and loops? I don't see anything in this interpreter to handle that.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:39):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:39):

abrown created PR Review Comment:

Yup, that makes more sense--changed them. I've been thinking a lot about how to interpret FP arithmetic correctly (e.g. apfloat, softfloat) so I started thinking that I would have to be very explicit everywhere.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:39):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:39):

abrown created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:41):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:41):

abrown created PR Review Comment:

Yes, we should remove this once the old x86 backend goes away. I only had to enable this because otherwise somewhere in the CI a non-x86 build is triggered that doesn't contain all of the Opcode::X86... variants, causing compilation failures.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:57):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 01 2020 at 22:57):

abrown created PR Review Comment:

Good point. I think I was running into lifetime issues and went the static method route to resolve them. I've changed them back now to &mut self.

Wasmtime GitHub notifications bot (Nov 01 2020 at 23:20):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 01 2020 at 23:20):

abrown created PR Review Comment:

Yup... this was left a bit unfinished because I was (and am still) debating whether this should be a tree of traits instead of a single level. For example, State could contain fn iflags() -> impl Flags<IntCC> and then separately I could define has/set/clear for Flags. Why I haven't done this? I'm concerned that, since impl Flags is likely not correct (though it expresses what I mean), I will be caught up in dyn issues, possible lifetime issues, mutable vs. non-mutable issues, etc. Do you have an opinion on this?

Wasmtime GitHub notifications bot (Nov 01 2020 at 23:36):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Nov 02 2020 at 03:30):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Nov 02 2020 at 04:53):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Nov 02 2020 at 04:56):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 04:56):

abrown created PR Review Comment:

Ok, I replaced the macros with closures in 5a121dd. I like the idea of closures better than macros but it made a few things a bit harder to read so I left it as a separate commit in case we want to revert.

Wasmtime GitHub notifications bot (Nov 02 2020 at 05:01):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 05:01):

abrown created PR Review Comment:

In the constant folding code, I added a PhiValues structure that scans through a function twice, first to capture all block parameters and secondly to map the SSA values that could be inputs to those block parameters (e.g. from a jump). I perform lookups into that mapping when I need to join. Perhaps there is something like PhiValues somewhere but I couldn't find it?

Wasmtime GitHub notifications bot (Nov 02 2020 at 17:41):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 17:41):

fitzgen created PR Review Comment:

FYI, traits can't contain -> impl Trait methods, you would have to have an associated type that is returned instead:
trait State {
    type IFlags: IFlagsState;

    fn iflags(&mut self) _> &mut Self::Iflags;

    // etc...
}
I generally don't think it is worth it to do this sort of thing unless there is code that will be generic over just IFlagsState and not the whole State, which I suspect isn't the case here.

Wasmtime GitHub notifications bot (Nov 02 2020 at 17:45):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 17:45):

fitzgen created PR Review Comment:

We don't have anything generic AFAIK, but you could take a look at the LICM and GVN implementations and see if they have anything we can factor out.

I must admit that I'm more interested in the design of the trait though. Don't have anything off the top of my head, just think it will need something like a join or meet method depending which way we are modeling our lattices, as well as a "should I take both branches?" predicate.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:03):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:03):

abrown created PR Review Comment:

I'll upstream that constant folding after this PR is merged and we can iterate on the design.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:13):

abrown updated PR #2323 from generic-interpreter to main:

This change re-implements the Cranelift interpreter to use generic values; this makes it possible to do abstract interpretation of Cranelift instructions. In doing so, the interpretation state is extracted from the Interpreter structure and is accessed via a State trait; this makes it possible to not only more clearly observe the interpreter's state but also to interpret using a dummy state (e.g. ImmutableRegisterState). This addition made it possible to implement more of the Cranelift instructions (~70%, ignoring the x86-specific instructions).

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:14):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 19:14):

abrown created PR Review Comment:

:+1: I documented it better and left it as-is.

Wasmtime GitHub notifications bot (Nov 02 2020 at 20:28):

abrown merged PR #2323.

Last updated: Dec 23 2024 at 12:05 UTC