Skip to content

Enable -sASYNCIFY + -sMEMORY64=2 #19913

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

Merged
merged 1 commit into from
Jul 27, 2023
Merged

Enable -sASYNCIFY + -sMEMORY64=2 #19913

merged 1 commit into from
Jul 27, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jul 27, 2023

It turns out that simply running the memory64 lowering pass after the asyncify pass is enough to make this work.

It turns out that simply running the memory64 lowering pass after
the asyncify pass is enough to make this work.
@sbc100 sbc100 requested a review from kripken July 27, 2023 11:14
@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 27, 2023

@dakenf

@sbc100 sbc100 enabled auto-merge (squash) July 27, 2023 16:00
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Nice this works so easily... I see we already made Asyncify track the pointer type of its memory, so makes sense it should work to run Asyncify first. I am slightly curious why running it second fails...

@sbc100 sbc100 merged commit 02b163b into main Jul 27, 2023
@sbc100 sbc100 deleted the wasm64l_asyncify branch July 27, 2023 17:17
@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 27, 2023

Nice this works so easily... I see we already made Asyncify track the pointer type of its memory, so makes sense it should work to run Asyncify first. I am slightly curious why running it second fails...

The asyncify pass creates a bunch of helper functions (and a global) that use the current pointer size (based on the index size of the memory), and we want that pointer size to be i64 ... even when using the memory64 lowering pass. This is because the API used by -sMEMORY64=2 with the outside world is still "pointers are i64". The lowering pass is designed only to effect the loads and stores. Any function that take pointers should use i64 and any global that holds a pointer should be i64.. even after lowering. If we lower first, then asyncify assumes (wrongly) that pointers are i32.

@kripken
Copy link
Member

kripken commented Jul 27, 2023

I see, thanks. We could make it track pointer types for those globals, I guess, but this seems like a good enough solution.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 27, 2023

I see, thanks. We could make it track pointer types for those globals, I guess, but this seems like a good enough solution.

Those globals do track the pointer type, but the problem is that after wam64 lowering binaryen's method of detecting what the pointer type of the program is no longer works and gives the wrong result. This is because the pointer type stays constant (i64) but the memory index is converted to i32.

Note: The lowering pass does not change the pointer type per-say, but just adds pointer truncation to every load/store.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 27, 2023

One option would be to add a "--force-pointer-type=i64" to binaryen to override its auto-detection. But doing the lowering last seems simpler.

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