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

[wasm] Expand interpreter PackedSimd support #87719

Closed
wants to merge 9 commits into from

Conversation

kg
Copy link
Member

@kg kg commented Jun 17, 2023

This PR refactors how PackedSimd intrinsics are defined in the interpreter (to simplify them) and expands the set of them that are supported.

I still need to change the lookup into a binary search, which will require doing a thread-safe sort of the table at first use or something, haven't gotten to that yet.

kg added 4 commits June 16, 2023 22:34
Rearrange cases

Add fp version of some intrinsics
Add some bitwise intrinsics

Refactorings

Cleanup

Add more packedsimd comparisons

Checkpoint

Checkpoint

Prototype implementation

Checkpoint

Checkpoint
Add more wasm opcodes
@kg kg added arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono labels Jun 17, 2023
@ghost ghost assigned kg Jun 17, 2023
@ghost
Copy link

ghost commented Jun 17, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR refactors how PackedSimd intrinsics are defined in the interpreter (to simplify them) and expands the set of them that are supported.

I still need to change the lookup into a binary search, which will require doing a thread-safe sort of the table at first use or something, haven't gotten to that yet.

Author: kg
Assignees: -
Labels:

arch-wasm, area-Codegen-Interpreter-mono

Milestone: -

@kg
Copy link
Member Author

kg commented Jun 20, 2023

I spent a bit trying to figure out a good way to use binary search for this and it requires introducing a mutex and some other stuff so I'm not sure it's worth it. Marking ready for review, I haven't found anything else wrong with it.

@kg kg marked this pull request as ready for review June 20, 2023 00:10
@kg kg requested a review from radekdoulik June 20, 2023 00:10
@vargaz
Copy link
Contributor

vargaz commented Jun 20, 2023

See simd-intrinsics.c for examples on how to define and search these tables.

@kg
Copy link
Member Author

kg commented Jun 20, 2023

See simd-intrinsics.c for examples on how to define and search these tables.

The problem is that one table entry doesn't match one method, it's 1 entry to many methods. So I can't just binary search for (name, argtype)

@BrzVlad
Copy link
Member

BrzVlad commented Jun 21, 2023

Couldn't we have the intrinsics defined in sorted by name order and then just do a binary search based on name ? And then also probe that entry and all the neighbouring entries as long as they have the same name. Should be easy enough and the lookup pretty much as fast as it can get.

@kg
Copy link
Member Author

kg commented Jun 21, 2023 via email

@kg
Copy link
Member Author

kg commented Jun 21, 2023

Stumped about why the qsort is crashing like this. I can repro locally, but only in one of my test cases and not when running test suites.

@kg
Copy link
Member Author

kg commented Jun 21, 2023

Stumped about why the qsort is crashing like this. I can repro locally, but only in one of my test cases and not when running test suites.

I think it's fixed now and is using the algorithm mentioned before (binary search by name, then scan forward and back for a match)

@kg
Copy link
Member Author

kg commented Jun 30, 2023

Closing in favor of #87903

@kg kg closed this Jun 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants