Only export functions with default visibility

[dschuff]

As discussed in #585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).

[kripken]

lgtm

[benvanik]

Any chance of this landing? It'd be nice to not export the world :)

[dschuff]

Ah yeah.... so we've been trying to bring up the remaining emscripten test suite tests for the wasm backend (which exercise a lot of emscripten features and, JS glue, etc) before landing this to see what kind of effect it had. But we are getting close on that, and we are also working on getting support in LLVM for wasm binaries (which will be a bit later), so now might be a good time to start thinking a little more about this as an ABI/convention issue.

[benvanik]

That makes sense - tests are good :)
Today wasm-opt can't do any dead function elimination and almost all inlining is prevented on code coming from clang, as s2wasm acting as the linker pins everything as an export. With this PR patched in my binaries get much much better.
Do you see the flow (clang -> s2wasm -> wasm-opt -> wasm-as) changing with LLVM? (guessing maybe clang -> lld -> wasm-opt -> wasm-as?) If lld does the linking and wasm-opt only ever sees things that really are exported that seems fine (I'm assuming from what I've seen discussed that wasm-opt is still used in that flow instead of/in addition-to the llvm optimizer - is that correct?)

[dschuff]

I think we'd want wasm-opt to be optional, rather than a requirement. the LLVM wasm backend has plenty of optimizations that it inherits from LLVM's target-independent code, so it produces pretty decent code quality. I could certainly imagine that wasm-opt might be able to improve on that, but we haven't done really any meaningful performance work yet.

[kripken]

wasm-opt should certainly be optional, but the benefit is noticeable already. It shrinks wasm-backend outputs by 3-4%, and could do more if we exported less by this PR.

That's for code size. For throughput, the wasm backend emits slower code than asm2wasm+wasm-opt, I believe on all the emscripten benchmark suite (except for one or two of the microbenchmarks where it's the same). I didn't try to run wasm-opt on the wasm backend's output - I think for that to work well, it would need to re-SSA it first. I started to write a PR with a pass for that, but haven't had time to finish it.

[dschuff]

Yeah; as I said we've basically been trying to mature the backend in the face of all the changes to wasm itself (and catch up to JSBackend's several-year head start). There are lots of optimization opportunities still on the table.

[kripken]

Do we still need this, or can it be closed?

[dschuff]

I think the reason I was holding off on this was to get the rest of the emscripten testsuite passing (which @jgravelle-google has been doing, along with the various general improvements like the libc update). At that point we would be more assured that turning this on wouldn't break anything.

More generally I think we still want to do this anyway, but the design for what "visibility" means (to the linker and in C++ code) should probably be done in conjunction with the other design conventions e.g. https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md and https://github.com/WebAssembly/tool-conventions/blob/master/DynamicLinking.md

So, short answer is, we can probably try rebasing it and turning it on more-or-less as-is shortly. If things are fine for the current emscripten+wasm-backend style of integration, then we should land it. If not, then we can close it and rethink things.

[guybedford]

Perhaps this would be worth landing behind a command line flag, if there is reservation to merging this? That way we can at least start running with the use cases. I'd really like to adopt this approach in my own compilation test workflows.

[sunfishcode]

I'm now working on replacing s2wasm with a different approach for connecting LLVM output to binaryen -- LLVM now has the ability to emit wasm binaries directly, so with some extra bookkeeping, we can skip the .s parsing step, which is fairly fragile due to not being a "real" .s parser.

That said, this is a fairly big project, so if people want to continue maintaining s2wasm in its current form, they're welcome to.

[guybedford]

@sunfishcode thanks so much for your response, is https://github.com/llvm-mirror/clang/blob/master/test/Driver/wasm-toolchain.c the best place to start to try and follow this direct wasm output from llvm?

[sunfishcode]

If you're looking for a specific file, https://github.com/llvm-mirror/llvm/blob/master/lib/MC/WasmObjectWriter.cpp does a lot of the work of the actual wasm object writing.