Skip to content

Run binaryen wasm-opt on wasm backend output #6029

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 7 commits into from
Jan 5, 2018
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Jan 5, 2018

This proposes we run the binaryen optimizer on wasm-backend output. It makes that output 5-10% smaller. This came up again today when looking at the clang running in the browser demo, which is from the wasm backend, and part of the reason for the large size is not running binaryen opts.

Debatable whether this makes sense to do by default or not - in -O1 and above it shrinks the output but increases build time, it's a tradeoff - but I lean towards emcc doing what emits good code. That clang build is adding binaryen to the build process, but emcc could do it by default so other people don't miss it. Also, there is no change to the speed of -O0 builds for fast iteration.

This PR also contains some closure fixes I noticed when running the test suite locally. I'm surprised these weren't noticed on the bots, maybe they don't have java and so skip closure, I guess?

@saschanaz
Copy link
Collaborator

saschanaz commented Jan 5, 2018

maybe they don't have java and so skip closure, I guess?

No, it installs java so it shouldn't be the case. (I remember there are some tests that fail if there is no java.)

@kripken
Copy link
Member Author

kripken commented Jan 5, 2018

Sorry, I should have been clearer, I meant the wasm bot, which runs latest llvm+wasm backend stuff, https://wasm-stat.us/console , not the CI we have here (which doesn't test the wasm backend yet).

@jgravelle-google
Copy link
Contributor

We do run closure on the wasm waterfall, which is currently red because of it.
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.wasm.llvm%2Flinux%2F27799%2F%2B%2Frecipes%2Fsteps%2FExecute_emscripten_testsuite__emwasm_%2F0%2Fstdout
This is something I was looking into, but had gotten far enough to realize I didn't know what to do with the error it was giving me and was about to start bisecting. So, thank you. Trying this patch locally this fixes the issue.

@dschuff
Copy link
Member

dschuff commented Jan 5, 2018

+1, I'd favor going with better code for now. I'd like to get to some backend optimization work in LLVM before too long, and hopefully that way we can improve code quality without the cost of serializing through another tool.

@jgravelle-google
Copy link
Contributor

I forgot to mention explicitly, but also +1 for optimization

@kripken kripken merged commit 379d949 into incoming Jan 5, 2018
@kripken kripken deleted the wasm-opt-wasm-backend branch January 5, 2018 18:05
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.

4 participants