Skip to content

Various cranelift interpreter improvements#6176

Merged
afonso360 merged 3 commits intobytecodealliance:mainfrom
bjorn3:bsc-unwinding-interpreter-improvements
Apr 7, 2023
Merged

Various cranelift interpreter improvements#6176
afonso360 merged 3 commits intobytecodealliance:mainfrom
bjorn3:bsc-unwinding-interpreter-improvements

Conversation

@bjorn3
Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 commented Apr 7, 2023

This is part of the changes required to use cranelift-interpreter in cg_clif.

bjorn3 added 3 commits April 7, 2023 14:47
This is necessary to create your own interpreter that reuses most of
cranelift-interpreter. For example to use a different State
implementation.
@bjorn3 bjorn3 requested a review from a team as a code owner April 7, 2023 14:52
@bjorn3 bjorn3 requested review from cfallin and removed request for a team April 7, 2023 14:52
Copy link
Copy Markdown
Contributor

@afonso360 afonso360 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This all looks good to me.

As far as I can understand these changes allow you to have your own State implementation for use in cg_clif (Awesome! 🎉). That being said we have some plans for removing this trait based approach for the interpreter (#5793).

It would be nice to know what we need to continue supporting the cg_clif use case (Which is something I'm definitely interested in helping with) when (if?) we move away from this trait based approach.

None of that blocks this, It's just something that I remembered might be an issue in the future.

@afonso360 afonso360 added this pull request to the merge queue Apr 7, 2023
Merged via the queue into bytecodealliance:main with commit bada17b Apr 7, 2023
@bjorn3
Copy link
Copy Markdown
Contributor Author

bjorn3 commented Apr 7, 2023

#5793 is for the Value type generic, right? Not for the State trait. In cg_clif's usage I give the interpreted code direct access to the compiler's address space. For this I implemented the checked_load and checked_store methods of the State trait. I also needed to change a couple of other methods compared to InterpreterState to allow accessing data objects, to allow directly reading functions and data objects from a Module implementation that can be serialized and deserialized and to allow specifying arbitrary rust code to handle libc function calls. The last thing depends on a cranelift-interpreter change I haven't opened a PR for yet.

As for the exact use case of cg_clif, currently I used it to prototype exception support in cranelift (for my bachelor thesis) without having to immediately implement it in any backend. This allowed me to discover several optimization passes that needed changes. But even outside this I will probably want to land support for the interpreter in cg_clif.

@bjorn3 bjorn3 deleted the bsc-unwinding-interpreter-improvements branch April 7, 2023 15:57
@afonso360
Copy link
Copy Markdown
Contributor

Oh, that's right! I had it in my mind that it was about both since the only other user of State was ImmutableRegisterState which was used for SCCP (I think!), but it looks like that issue only covers Value.

I think we should probably revisit this when we start making progress on #5793, just to make sure we can keep folks happy. Thanks for sharing!

As for the exact use case of cg_clif, currently I used it to prototype exception support in cranelift (for my bachelor thesis) without having to immediately implement it in any backend. This allowed me to discover several optimization passes that needed changes. But even outside this I will probably want to land support for the interpreter in cg_clif.

Wow! This is an awesome use case! I look forward to reading that! 🚀

brendandburns pushed a commit to brendandburns/wasmtime that referenced this pull request Apr 13, 2023
* Remove the validate_address State trait method

It isn't used anywhere

* Expose the inner Function of a Frame

This is necessary to create your own interpreter that reuses most of
cranelift-interpreter. For example to use a different State
implementation.

* Support the symbol_value and tls_value instructions in the interpreter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants