Add initial Cranelift interpreter #1223

abrown · 2020-03-04T04:17:45Z

abrown · 2020-03-04T04:41:24Z

@bnjbvr, I had to commit 1bed1db to get this back in a working state. If the barrier to merging is that we only want to expose features that are complete, we could remove the clif-util interpret ... functionality. We have to start somewhere, though, so I recommend we merge this as-is--missing instructions and all--and add a log message to clif-util interpret ... like This is an unstable feature and a work in progress implementation.

github-actions · 2020-03-12T17:11:28Z

Subscribe to Label Action

This issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t"

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

Learn more.

github-actions · 2020-03-12T17:17:56Z

Subscribe to Label Action

This issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t"

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

Learn more.

github-actions · 2020-03-12T17:17:58Z

Subscribe to Label Action

This issue or pull request has been labeled: "c", "r", "a", "n", "e", "l", "i", "f", "t"

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

Learn more.

github-actions · 2020-03-17T10:52:03Z

Subscribe to Label Action

This issue or pull request has been labeled: "cranelift"

Users Subscribed to "cranelift"

@bnjbvr

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

Learn more.

bnjbvr

A few comments below; I really have looked only at the first commit, and realized it wouldn't compile locally (because the parser module doesn't exist as of this commit, it seems to be introduced later). I think the commits need to be rearranged; it'd be nice if changes introduced by the latest commit could also be absorbed in the relevant commits that introduced the lines they're changing (it would definitely help the review!).

I'm happy to give it a second look once my first comments have been addressed. When this is ready, please re-request a new review, and I'll do my best to do it promptly this time (sorry about the slow review time!). Cheers!

cranelift-interpreter/Cargo.toml

cranelift-interpreter/src/runner.rs

cranelift-interpreter/src/interpreter.rs

bnjbvr · 2020-03-30T15:26:42Z

cranelift-interpreter/src/interpreter.rs

+                ControlFlow::ContinueAt(ebb, old_names) => {
+                    let new_names = frame.function.dfg.ebb_params(ebb);
+                    frame.rename(&old_names, new_names);
+                    return self.block(frame, ebb); // TODO check that TCO happens


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

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?

cranelift-interpreter/src/interpreter.rs

abrown · 2020-04-22T01:02:49Z

@bnjbvr, I think is ready for another review but it would be best if we took a look at #1517 first since this uses some of the changes that PR made to RunCommand.

bnjbvr · 2020-05-05T09:38:00Z

Sorry, I am swamped in reviews again, and I don't know when I'd be able to do this; forwarding to other people.

fitzgen

Looks like a good start on an interpreter! Happy to land this now and then iterate on it further in-tree. Some suggestions inline below as well -- feel free to merge once they are addressed without re-requesting review.

It would be cool if rather than panicking on unknown opcodes, we returned an error that could be matched upon. Then we would be able to do things like differential fuzzing between the interpreter and compiler (with an early exit if the interpreter doesn't support sine particular opcode yet).

cranelift/interpreter/src/environment.rs

fitzgen · 2020-05-05T21:20:42Z

cranelift/interpreter/src/frame.rs

+    /// any old references that are not in `old_names`. TODO This performs an extra allocation that
+    /// could be removed if we copied the values in the right order (i.e. when modifying in place,
+    /// we need to avoid changing a value before it is referenced).
+    pub fn rename(&mut self, old_names: &[ValueRef], new_names: &[ValueRef]) {


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?

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.

cranelift/interpreter/src/interpreter.rs

Also, returns a `Result` in the `RunCommand::run` helper.

This is an incomplete version of a Cranelift IR interpreter: only a small subset of instructions are implemented and (known) missing parts are marked with TODO or FIXME.

abrown · 2020-05-07T23:50:58Z

Stuff I'm going to make a note to work on later:

a Trap variant for unknown opcodes instead of using unimplemented!, from @fitzgen
convert the block() method to use a loop instead of recursing, from @bnjbvr
think about renaming Frame and Environment, from a comment to @fitzgen

abrown requested a review from bnjbvr March 4, 2020 04:17

github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 11, 2020

abrown force-pushed the upstream-interpreter-in-wasmtime branch 2 times, most recently from d4d8432 to 0923bb5 Compare March 28, 2020 00:16

bnjbvr reviewed Mar 30, 2020

View reviewed changes

abrown mentioned this pull request Mar 30, 2020

Parse run commands #1436

Merged

krk mentioned this pull request Apr 4, 2020

Print return values of run tests if type is not boolean. bytecodealliance/cranelift#1231

Closed

abrown force-pushed the upstream-interpreter-in-wasmtime branch from 0923bb5 to f3287d6 Compare April 21, 2020 22:02

github-actions bot added the cranelift:meta Everything related to the meta-language. label Apr 21, 2020

abrown force-pushed the upstream-interpreter-in-wasmtime branch 4 times, most recently from 199342d to d4f9147 Compare April 21, 2020 23:51

bnjbvr self-requested a review April 22, 2020 10:12

abrown force-pushed the upstream-interpreter-in-wasmtime branch from d4f9147 to 8c6d933 Compare May 1, 2020 04:08

bnjbvr requested review from fitzgen and sunfishcode and removed request for bnjbvr May 5, 2020 09:38

fitzgen approved these changes May 5, 2020

View reviewed changes

abrown added 4 commits May 7, 2020 15:31

Cast DataValues to and from native types

ce7c921

Also, returns a `Result` in the `RunCommand::run` helper.

Add a CLIF interpreter

ae4b77d

This is an incomplete version of a Cranelift IR interpreter: only a small subset of instructions are implemented and (known) missing parts are marked with TODO or FIXME.

Add test interpret support to filetests

2b75c00

Move iterate_files to the utils module

214d8c6

Add an interpret command to clif-util

de27153

abrown force-pushed the upstream-interpreter-in-wasmtime branch from 8c6d933 to de27153 Compare May 7, 2020 22:46

abrown merged commit b65bd1c into bytecodealliance:master May 7, 2020

abrown deleted the upstream-interpreter-in-wasmtime branch May 7, 2020 23:51

abrown mentioned this pull request May 26, 2020

Avoid recursion in Interpreter::block #1754

Merged

Add initial Cranelift interpreter #1223

Add initial Cranelift interpreter #1223

Uh oh!

Conversation

abrown commented Mar 4, 2020

Uh oh!

abrown commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 12, 2020

Subscribe to Label Action

Uh oh!

github-actions bot commented Mar 12, 2020

Subscribe to Label Action

Uh oh!

github-actions bot commented Mar 12, 2020

Subscribe to Label Action

Uh oh!

github-actions bot commented Mar 17, 2020

Subscribe to Label Action

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

abrown Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abrown commented Apr 22, 2020

Uh oh!

bnjbvr commented May 5, 2020

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fitzgen May 5, 2020

Choose a reason for hiding this comment

Uh oh!

abrown May 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abrown commented May 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abrown commented Mar 4, 2020 •

edited

Loading