Handle EM_ASM/EM_JS in LLVM wasm backend O0 output #1888
Conversation
jgravelle-google
left a comment
jgravelle-google
left a comment
There was a problem hiding this comment.
I assume this never worked? How has this never worked. Have we always been compiling with some minimal level of LTO as part of bitcode linking? Because we weren't even running wasm-opt on the llvm backend path in emscripten until after this code was written.
src/wasm/wasm-emscripten.cpp
Outdated
| if (block && block->list.size() > 0) { | ||
| first = block->list[0]; | ||
| // skip a potential initial nop | ||
| if (first->is<Nop>()) { |
There was a problem hiding this comment.
Are the nops because of the simplify-locals pass? Would we expect to get more than one?
I'm completely okay with leaving this as-is until we have a need a for loop here, because this is simpler / easier to read.
There was a problem hiding this comment.
Yes, that pass will replace the initial set with a single nop. There won't be anything more here since there's just one constant in the whole function.
| } | ||
| if (addrConst == nullptr) { | ||
| Fatal() << "Unexpected generated __em_js__ function body: " << curr; | ||
| Fatal() << "Unexpected generated __em_js__ function body: " << curr->name; |
There was a problem hiding this comment.
I thought << curr printed the function body. Is that << *curr or does that not actually exist?
The thought being that printing the unexpected code inline with the error would be the information a user would really want, vs. the name which lets one look up the code later via wasm-dis.
There was a problem hiding this comment.
Hmm, not sure what *curr does for a function. For an AST node, that would print in out. Here it was just printing the pointer until this PR, and now it prints the name (which seems sufficient for debugging to me at least ;)
|
I switched the pass to the Alternatively I guess we could maybe write a custom analysis for these locals, just walk the function, note locals assigned to once, etc. That might be better for debugging of O0 code somehow? |
|
Seems fine to me, I think custom analysis would be overkill, though it would probably have fewer side-effects on the rest of the module. Alternatively we could teach the EM_JS pass to understand the O0 patterns, because it will pretty much always be one or the other, if that's simpler and not too fragile. |
|
I experimented with adding custom logic for this. It basically means tracking set/get pairs in basic blocks, and is actually pretty simple, let me know what you think of the current code in this PR. I lean towards this approach, because otherwise running an optimization pass may have bad downsides for debug info in |
jgravelle-google
left a comment
jgravelle-google
left a comment
There was a problem hiding this comment.
Nice, that seems best to me too
Two fixes were needed: * Update binaryen which now handles EM_ASM and EM_JS in that case, see WebAssembly/binaryen#1888 * Disable test_biggerswitch which OOMs in nodejs at -O0 (unnoticed before since we ran -O2 for codegen, see #7928 (comment) )
See emscripten-core/emscripten#7928 - we have been optimizing all wasms until now, and noticed this when the wasm object file path did not do so. When not optimizing, our methods of handling EM_ASM and EM_JS fail since the patterns are different.
Specifically, for EM_ASM we hunt for
emscripten_asm_const(X,where X is a constant, but without opts it may be a get of a local. Runningsimplify-locals-nostructureis a bit of a big hammer here, and will slow down compilation time, slightly, but the alternative is to do some local analysis which would add complexity and time as well.For EM_JS we look for a function with a constant returned in it. Without opts, this might be a set to a local, then a return of a get.
simplify-locals-nostructurewhich we run for EM_ASMs fixes the local issue, and here we also need to look past the nop (that the set is replaced with) and into the return value.