Skip to content
This repository was archived by the owner on Nov 1, 2021. It is now read-only.

Em asm calltype#196

Merged
juj merged 2 commits intoemscripten-core:incomingfrom
juj:em_asm_calltype
Nov 22, 2017
Merged

Em asm calltype#196
juj merged 2 commits intoemscripten-core:incomingfrom
juj:em_asm_calltype

Conversation

@juj
Copy link
Collaborator

@juj juj commented Aug 23, 2017

This expands EM_ASM handling to allow specifying how the JS code is run in multithreaded environment: a) locally (synchronously) on the calling thread, b) synchronously proxied to main browser thread, or c) asynchronously proxied to main browser thread.

This needs a matching Emscripten side PR to land, I'll get to that at some point.

// EM_ASM support

std::string handleAsmConst(const Instruction *CI) {
// callType is one of "", "sync_on_main_thread_" or "async_on_main_thread_" and specifies
Copy link
Member

Choose a reason for hiding this comment

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

how about an enum for the call type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Enum sounds fine, I'll change to that.

@kripken
Copy link
Member

kripken commented Aug 23, 2017

Is there a writeup of the direction this is going? Stuff like this would need to happen in the wasm backend too, so deserves a wide discussion.

@juj juj force-pushed the em_asm_calltype branch from 5a4f21c to 04f33e4 Compare August 25, 2017 07:53
@juj
Copy link
Collaborator Author

juj commented Aug 25, 2017

I'm quite complete with the multithreading work now, and this PR ended up being the only change needed on the LLVM side. The remaining changes were mostly to jsifier, which processes the proxying model for JS libraries. The model is the same for JS libraries and EM_ASM blocks. I'll ping on this again when I have the Emscripten side PR cleaned up for looks.

@juj
Copy link
Collaborator Author

juj commented Nov 16, 2017

This PR should now be good to merge. Updated this along with matching emscripten-core/emscripten#5544 to address review comments. CC @kripken, @jgravelle-google .

@kripken
Copy link
Member

kripken commented Nov 16, 2017

No concerns from me.

@jgravelle-google
Copy link
Contributor

Looks reasonable to me

@juj
Copy link
Collaborator Author

juj commented Nov 22, 2017

Perfect!

@juj juj merged commit 439d206 into emscripten-core:incoming Nov 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants