Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Jul 10, 2024

Before this PR, if we did -O3 then we ended up doing this:

  • wasm-opt -O3
  • wasm-metadce
  • wasm-opt --minify-imports-and-exports

The first invocation uses StackIR (which is enabled by default in -O3), but then the
later invocations "lose" those optimizations, since they read the wasm binary and
generate structured BinaryenIR once more. We would need to do StackIR opts again,
but even more, we'd need to do work to undo the effects of stackiness, which can
lead to more locals and such, see #22196

After this PR, we do this:

  • wasm-opt -O3 --no-stack-ir
  • wasm-metadce
  • wasm-opt --minify-imports-and-exports --optimize-stack-ir

The first command is now told to not use StackIR. The second remains unchanged.
The last then does StackIR optimizations.

This is the same amount of work (we just shuffle around the StackIR opts), so
compile times are unaffected, but we gain some code improvements. To see them,
see the commit before last which applies the size changes on top of a recent rebase.
Many tests improve by small but noticeable amounts, though two wasm2js tests
somehow lose by a few bytes, which I think is noise.

There is a slight code complexity downside here, but given this avoids corner
cases that may be quite annoying (#22196) it seems worthwhile.

Depends on WebAssembly/binaryen#6725 to land first.

@kripken
Copy link
Member Author

kripken commented Jul 10, 2024

Note that we'd see even more code size improvements here, but the benefits of this are missed in the metadce tests. Those tests run wasm-opt to strip the names section, and that roundtrip undoes StackIR benefits. Perhaps I should make the test strip the names using llvm-objcopy first (which I see code for in building.py), what do you think @sbc100 ?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 10, 2024

Note that we'd see even more code size improvements here, but the benefits of this are missed in the metadce tests. Those tests run wasm-opt to strip the names section, and that roundtrip undoes StackIR benefits. Perhaps I should make the test strip the names using llvm-objcopy first (which I see code for in building.py), what do you think @sbc100 ?

So calling wasm-opt repeatedly somehow means we loose these benefits forever?

@kripken
Copy link
Member Author

kripken commented Jul 10, 2024

Yes, StackIR opts generate "stacky" wasm forms, that do not fit perfectly in BinaryenIR. Binaryen may need to add a local or a block or such to handle them. We can gain the benefits again if we re-optimize (to remove the locals, and to regenerate StackIR).

But it is simplest to just run StackIR at the very end, because that is when it actually matters. StackIR opts are just minor binary format tweaks that do not unlock any other benefits.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 10, 2024

Looks like this needs a rebase.

@kripken
Copy link
Member Author

kripken commented Jul 10, 2024

Updated after the metadce test change. Test results here now show more benefits to code size on those metadce tests.

tools/link.py Outdated
if settings.WASM_EXNREF:
passes += ['--emit-exnref']

# If we are not going to run metadce then we will invoke wasm-opt later for
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this say "If we are going to run.."?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

@kripken
Copy link
Member Author

kripken commented Jul 11, 2024

The last commit adds a fix that is needed to avoid a test breaking here. The test checked that we don't alter wasm size if we use --emit-symbol-map, and now that this PR enables StackIR opts then the old way we did that noticeably regressed wasm size. That is, the improvements in this PR were undone when that flag was used. To fix it, just use llvm-objcopy instead of wasm-opt to strip the names section.

@kripken kripken merged commit 3905550 into emscripten-core:main Jul 11, 2024
@kripken kripken deleted the no-stack-ir branch July 11, 2024 18:30
verhovsky pushed a commit to verhovsky/emscripten that referenced this pull request Jul 30, 2024
Before this PR, if we did -O3 then we ended up doing this:

* wasm-opt -O3
* wasm-metadce
* wasm-opt --minify-imports-and-exports

The first invocation uses StackIR (which is enabled by default in -O3), but then the
later invocations "lose" those optimizations, since they read the wasm binary and
generate structured BinaryenIR once more. We would need to do StackIR opts again,
but even more, we'd need to do work to undo the effects of stackiness, which can
lead to more locals and such, see emscripten-core#22196

After this PR, we do this:

* wasm-opt -O3 --no-stack-ir
* wasm-metadce
* wasm-opt --minify-imports-and-exports --optimize-stack-ir

The first command is now told to not use StackIR. The second remains unchanged.
The last then does StackIR optimizations.

This is the same amount of work (we just shuffle around the StackIR opts), so
compile times are unaffected, but we gain some code improvements. To see them,
see the commit before last which applies the size changes on top of a recent rebase.
Many tests improve by small but noticeable amounts, though two wasm2js tests
somehow lose by a few bytes, which I think is noise.

There is a slight code complexity downside here, but given this avoids corner
cases that may be quite annoying (emscripten-core#22196) it seems worthwhile.

Also improve our handling of --emit-symbol-map to use llvm-objcopy, which avoids
wasm-opt roundtrip issues (this is necessary in this commit as otherwise the
improvements of this PR are undone by that roundtrip, which was caught by a test).
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.

2 participants