Minimal JS legalization #1824
Minimal JS legalization #1824
Conversation
|
cc @awtcode |
| #include "wasm.h" | ||
|
|
||
| namespace wasm { | ||
|
|
There was a problem hiding this comment.
Is it the convention to add these extra newlines between namespace declaration? If not I've remove them.
There was a problem hiding this comment.
grep indicates they are the norm in this codebase. all places but one, in fact, I'll fix that.
|
Thanks for doing this @kripken :) Really appreciate it. What build flags should I use to ensure that only dynCalls are legalized? In my case, I believe the i64 exported functions are not called from JS as well so is there a way to turn off legalization for these functions? Thanks for your help. |
|
|
||
| namespace ABI { | ||
|
|
||
| enum LegalizationLevel { |
There was a problem hiding this comment.
Maybe this should be enum class so externally it has to be spelled ABI::LegalizationLevel::Full instead of just ABI::Full? Or alternatively rename Full to FullLegalization just for clarity?
| @@ -118,6 +131,11 @@ struct LegalizeJSInterface : public Pass { | |||
| return false; | |||
| } | |||
|
|
|||
| bool isRelevant(Export* ex, Function* func) { | |||
There was a problem hiding this comment.
how about naming this shouldLegalize or something like that?
|
@awtcode
I don't understand - what does "as well" refer to? (what else could call them aside from JS, that legalization matters for?) @dschuff sounds good, followups for your comments are in #1832 |
|
Sorry for the confusion @kripken. I was thinking about i64 functions that we export explicitly e.g. EMSCRIPTEN_KEEPALIVE and -s EXPORTED_FUNCTIONS. Since I won't be calling them from JS but only Wasm, will they still be legalized with LEGALIZE_JS_FFI=0? From what I understand, only dynCalls need to be legalized at this point in time. But I am fine with all explicitly exported functions being legalized at this point in time as the number is not large. Let me know if I need to provide more clarifications. |
|
@awtcode I understand, thanks. There's nothing special about exports. So if you build with legalization off, it will not legalize those (it will just legalize dynCalls, if there are any). |
|
Thanks for the clarification @kripken. |
|
@kripken , will you be releasing a new version of emsdk that contains this PR? |
|
Probably soon, there is current work on some ABI issues which will include this emscripten-core/emscripten-fastcomp#249 etc. |
Even when we don't want to fully legalize code for JS, we should still legalize things that only JS cares about. In particular,
dynCall_*methods are used from JS to call into the wasm table, and if they exist they are only for JS, so we should only legalize them.The use case motivating this is that in dynamic linking you may want to disable legalization, so that wasm=>wasm module calls are fast even with i64s, but you do still need dynCalls to be legalized even in that case, otherwise an invoke with an i64 parameter would fail.