Skip to content
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 IRBuilder in the binary parser #6963

Draft
wants to merge 1 commit into
base: binary-parser-refactor-name-fixup
Choose a base branch
from

Conversation

tlively
Copy link
Member

@tlively tlively commented Sep 21, 2024

IRBuilder is a utility for turning arbitrary valid streams of Wasm instructions into valid Binaryen IR. It is already used in the text parser, so now use it in the binary parser as well. Since the IRBuilder API for building each intruction requires only the information that the binary and text formats include as immediates to that instruction, the parser is now much simpler than before. In particular, it does not need to manage a stack of instructions to figure out what the children of each expression should be; IRBuilder handles this instead.

There are some differences between the IR constructed by IRBuilder and the IR the binary parser constructed before this change. Most importantly, IRBuilder generates better multivalue code because it avoids eagerly breaking up multivalue results into individual components that might need to be immediately reassembled into a tuple. It also parses try-delegate more correctly, allowing the delegate to target arbitrary labels, not just other trys. There are also a couple superficial differences in the generated label and scratch local names.

There are two remaining bugs: First, support for creating DWARF location spans is missing because IRBuilder does not have an API for that yet (but source map locations work fine). Second, IRBuilder generates pops inside nameless blocks in some circumstances involving stacky code. This is currently an IR validation error, so #6950 will have to be resolved before this can land.

This change also makes the binary parser significantly slower (by about 50%). The lowest hanging performance fruit seems to be tracking branch targets in IRBuilder to avoid having to scan for branches when finalizing blocks.

IRBuilder is a utility for turning arbitrary valid streams of Wasm
instructions into valid Binaryen IR. It is already used in the text
parser, so now use it in the binary parser as well. Since the IRBuilder
API for building each intruction requires only the information that the
binary and text formats include as immediates to that instruction, the
parser is now much simpler than before. In particular, it does not need
to manage a stack of instructions to figure out what the children of
each expression should be; IRBuilder handles this instead.

There are some differences between the IR constructed by IRBuilder and
the IR the binary parser constructed before this change. Most
importantly, IRBuilder generates better multivalue code because it
avoids eagerly breaking up multivalue results into individual components
that might need to be immediately reassembled into a tuple. It also
parses try-delegate more correctly, allowing the delegate to target
arbitrary labels, not just other `try`s. There are also a couple
superficial differences in the generated label and scratch local names.

There are two remaining bugs: First, support for creating DWARF location
spans is missing because IRBuilder does not have an API for that
yet (but source map locations work fine). Second, IRBuilder generates
pops inside nameless blocks in some circumstances involving stacky code.
This is currently an IR validation error, so #6950 will have to be
resolved before this can land.

This change also makes the binary parser significantly slower (by about
50%). The lowest hanging performance fruit seems to be tracking branch
targets in IRBuilder to avoid having to scan for branches when
finalizing blocks.
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.

1 participant