-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Use mainReadsParams metadata from wasm-emscripten-finalize #9064
Conversation
… (this change avoids the binaryen addition of the flags causing tests to break, as we assert on surprising flags)
src/postamble.js
Outdated
#endif | ||
]( | ||
#if MAIN_READS_PARAMS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky to read, because we're parameterizing on two #if
s and this is better than having 4 slightly-different copies of the same statement.
Would this still work if we had earlier
#else // not MAIN_READS_PARAMS
var argc = 1;
var argv = 0;
#endif
? Because then we can leave the _proxy_main/_main logic alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, yeah - it will add 0,0
to the code size that way, but that might be slightly better for the VM anyhow (to not have to coerce an undefined). I'll change it.
src/postamble.js
Outdated
#endif | ||
]( | ||
#if MAIN_READS_PARAMS | ||
argc, argv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This drops the 0, I assume that's fine? Presumably an undefined can get synthesized to a 0 in the case where we have a char* envp[]
in main
run_process([PYTHON, EMCC, 'yes.c', '-O3', '-o', 'yes.js']) | ||
yes = os.path.getsize('yes.js') | ||
# not having to set up argc/argv allows us to avoid including a | ||
# significant amount of JS for string support (which is not needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because metadce... that's really cool :)
…n-core#9064) This lets the wasm backend path not emit code to set up argc/argv if main doesn't need them. It also avoids that code bringing in some string and stack allocation support (metadce improvements are from the latter). This reduces --closure 1 -O3 size of hello world by almost 10%.
This lets the wasm backend path not emit code to set up argc/argv if main doesn't need them. It also avoids that code bringing in some string and stack allocation support (metadce improvements are from the latter).
This reduces
--closure 1 -O3
size of hello world by almost 10%.