Skip to content

Make cranelift-interpreter non-generic over value#6178

Merged
afonso360 merged 4 commits intobytecodealliance:mainfrom
bjorn3:bsc-unwinding-interpreter-non-generic
Apr 11, 2023
Merged

Make cranelift-interpreter non-generic over value#6178
afonso360 merged 4 commits intobytecodealliance:mainfrom
bjorn3:bsc-unwinding-interpreter-non-generic

Conversation

@bjorn3
Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 commented Apr 7, 2023

Fixes #5793

@bjorn3 bjorn3 requested a review from a team as a code owner April 7, 2023 17:03
@bjorn3 bjorn3 requested review from fitzgen and removed request for a team April 7, 2023 17:03
pub type ValueResult<T> = Result<T, ValueError>;

pub trait Value: Clone + From<DataValue> {
pub trait DataValueExt: Sized {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Left this as extension trait to make this PR smaller. We can always move methods to DataValue directly or inline them at the call site in cranelift-interpreter in the future.

}
fn le(&self, other: &Self) -> ValueResult<bool> {
Ok(other.eq(self)? || other.gt(self)?)
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These needed conflicted with PartialEq/PartialOrd impls of DataValue causing ambiguity during method resolution. I took the opportunity to inline them all.

pub trait Value: Clone + From<DataValue> {
pub trait DataValueExt: Sized {
// Identity.
fn ty(&self) -> Type;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method directly forwards to the equivalent method on DataValue.

pub trait Value: Clone + From<DataValue> {
pub trait DataValueExt: Sized {
// Identity.
fn ty(&self) -> Type;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method directly forwards to the equivalent method on DataValue.

@fitzgen
Copy link
Copy Markdown
Member

fitzgen commented Apr 10, 2023

@afonso360 or @abrown: do one of you want to review this one?

@afonso360
Copy link
Copy Markdown
Contributor

Yeah, I'll take a look at this.

@afonso360 afonso360 self-requested a review April 10, 2023 19:57
@afonso360 afonso360 self-assigned this Apr 10, 2023
Copy link
Copy Markdown
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

I'm excited about this so I took a quick look. It appears to be exactly the almost-mechanical changes necessary, which is exactly what I hoped to see.

@afonso360, I'd still appreciate your review just in case I missed something. Feel free to merge when you're ready.

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.

LGTM as well! Thanks for doing this, it has been slightly bothering me for a long time 😄 !

I have to head out now, but feel free to merge once the nit is addressed.

Co-authored-by: Jamey Sharp <jamey@minilop.net>
@jameysharp jameysharp added this pull request to the merge queue Apr 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2023
@afonso360
Copy link
Copy Markdown
Contributor

Looks like we are still missing one place where we use the generics version of DataValue

@bjorn3 bjorn3 requested a review from a team as a code owner April 11, 2023 09:51
@afonso360 afonso360 added this pull request to the merge queue Apr 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2023
@bjorn3
Copy link
Copy Markdown
Contributor Author

bjorn3 commented Apr 11, 2023

Updated the doc comments for the Value -> DataValueExt change.

@afonso360 afonso360 enabled auto-merge April 11, 2023 11:02
@afonso360 afonso360 added this pull request to the merge queue Apr 11, 2023
Merged via the queue into bytecodealliance:main with commit 96a60aa Apr 11, 2023
@bjorn3 bjorn3 deleted the bsc-unwinding-interpreter-non-generic branch April 11, 2023 11:52
brendandburns pushed a commit to brendandburns/wasmtime that referenced this pull request Apr 13, 2023
)

* Make cranelift-interpreter non-generic over value

Fixes bytecodealliance#5793

* Review suggestion

Co-authored-by: Jamey Sharp <jamey@minilop.net>

* Fix fuzz target

* Update doc comments

---------

Co-authored-by: Jamey Sharp <jamey@minilop.net>
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Apr 16, 2023
)

* Make cranelift-interpreter non-generic over value

Fixes bytecodealliance#5793

* Review suggestion

Co-authored-by: Jamey Sharp <jamey@minilop.net>

* Fix fuzz target

* Update doc comments

---------

Co-authored-by: Jamey Sharp <jamey@minilop.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cranelift: concretize the interpreter

4 participants