wasmtime / PR #1223 Add initial Cranelift interpreter · git-wasmtime

Out of curiosity, should all these modules be publicly exposed? Instead, could we expose the minimal amount that's required to be useful and for testing?

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:06):

bnjbvr created PR Review Comment:

nit: this field's name is a bit strange, I'd have expected a hashmap here.. How about func_names instead?

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:06):

bnjbvr created PR Review Comment:

Could the recursion be avoided by having an infinite loop above the for loop, remembering what the next BB will be, and breaking when the next BB differs from the current BB?

(also nit: we do have BB and not EBBs now :))

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:06):

bnjbvr created PR Review Comment:

brnz is actually polymorphic and can take integers as inputs. Can you either implement the right thing or add a TODO here, please?

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:06):

bnjbvr created PR Review Comment:

Could this function be inlined at uses instead?

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:06):

bnjbvr created PR Review Comment:

Alternatively, could the opcode be passed to binary, and the result can be computed there (with a match on the opcode), without passing a function pointer? Then we don't need the implicit add/sub impl of values, and it makes it easier to add new opcodes.

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:33):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Mar 30 2020 at 16:33):

bjorn3 created PR Review Comment:

Miri exposes a lot of things that are only used by priroda, which is a graphical debugger based on miri. It may make sense to do the same for cranelift-interpreter.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:02):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:13):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:14):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (Apr 21 2020 at 23:09):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (Apr 21 2020 at 23:51):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:00):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:00):

abrown created PR Review Comment:

The newly-pushed commits move all of this stuff into the filetests crate.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:05):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:06):

abrown created PR Review Comment:

Now that RunCommand is in cranelift-reader, I was able to remove the clif module and use that instead. The other modules we probably do want to make public: interpreter, frame, environment.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:07):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:07):

abrown created PR Review Comment:

In the rebased commits I used a macro instead; let me know what you think of that.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:58):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 00:58):

abrown created PR Review Comment:

The rebased commits have block instead of ebb. My TCO comment (now removed) was in hopes that Rust was somehow smart enough to avoid recursive calls... is there an easy way to check if that is happening?

Wasmtime GitHub notifications bot (Apr 22 2020 at 10:12):

bnjbvr requested bnjbvr for a review on PR #1223.

Wasmtime GitHub notifications bot (May 01 2020 at 04:08):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (May 05 2020 at 09:38):

bnjbvr requested fitzgen and sunfishcode for a review on PR #1223.

Wasmtime GitHub notifications bot (May 05 2020 at 09:38):

bnjbvr requested fitzgen and sunfishcode for a review on PR #1223.

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

Oh, I see. Yeah, I think the intent would be a lot more clear with a hash map.

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

It looks like this is actually mapping indices to names. If so, then the field name should be changed to index_to_function_name to reflect that.

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

This can be made a bit more concise and less boiler plate-y with ? propagation:
        let fr = self.index_of(name)?;
        self.get_by_func_ref(fr)

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

assert_eq!(old_names.len(), new_names.len());

Also, it isn't valid to clear every other value, since blocks are free to access variables defined in other blocks that dominate them, right?

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

Check that arguments.len() == parameters.len()?

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                    _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                    _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 05 2020 at 21:38):

fitzgen created PR Review Comment:

                _ => unimplemented!("interpreter doesn't support opcode yet: {:?}", opcode),

Wasmtime GitHub notifications bot (May 07 2020 at 22:10):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 22:10):

abrown created PR Review Comment:

Nice. Only thought that worked with Result!

Wasmtime GitHub notifications bot (May 07 2020 at 22:13):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 22:13):

abrown created PR Review Comment:

The assertion is inside Frame::with_parameters

Wasmtime GitHub notifications bot (May 07 2020 at 22:19):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 22:19):

abrown created PR Review Comment:

But those variables would be in another frame and I am using a frame per block; we can't access SSA values from another block (I just checked). I guess the naming is confusing because normally one would expect a "frame per function" scheme. I'll leave it for now but I'll try to think of some better name... BlockEnvironment? And rename Environment to something like FunctionEnvironment? Suggestions welcome.

Wasmtime GitHub notifications bot (May 07 2020 at 22:46):

abrown updated PR #1223 from upstream-interpreter-in-wasmtime to master:

See https://github.com/bytecodealliance/cranelift/pull/1351.

Wasmtime GitHub notifications bot (May 07 2020 at 23:51):

abrown merged PR #1223.

Last updated: Dec 23 2024 at 12:05 UTC