Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmtime: Implement table.fill #1973

Merged
merged 1 commit into from
Jul 6, 2020
Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jul 3, 2020

Part of #929

@fitzgen fitzgen requested a review from alexcrichton July 3, 2020 00:00
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Could you add a fill method to the wasmtime crate too? Also how come there are two builtin functions indices but only one actual function?

@fitzgen
Copy link
Member Author

fitzgen commented Jul 6, 2020

Could you add a fill method to the wasmtime crate too?

I'd prefer to make a separate PR for that, and I already have a separate checkbox for bubbling these things out to the API in the tracking issue so it shouldn't get lost or forgotten about.

Also how come there are two builtin functions indices but only one actual function?

This is the same as our table.grow implementation: they methods have the same Rust signature and essentially do the same thing regardless which reference type they are working with but Cranelift needs to have different signatures for the different variants so that it can produce stack maps for values flowing into the externref variants. We don't have much to gain by making different copies of the functions because they are already out-of-line, relatively expensive calls doing expensive operations, and we can have less code (in terms of binary size and maintenance) by having one function.

@fitzgen fitzgen merged commit 80ff22f into bytecodealliance:main Jul 6, 2020
@fitzgen fitzgen deleted the table-fill branch July 6, 2020 16:03
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