-
Notifications
You must be signed in to change notification settings - Fork 791
Add initial support for compact import section. NFC #2685
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
Conversation
27733a7 to
66515ca
Compare
7df3a9e to
e3ba87d
Compare
e3ba87d to
a75cde2
Compare
bvisness
left a comment
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.
All looks reasonable to me, although I've not looked at WABT's code before.
7df27ea to
a4bda2c
Compare
753bf18 to
3720508
Compare
See https://github.com/WebAssembly/compact-import-section For this initial commit I've just added a single simple test in form of test/binary/compact-imports.txt. Once we update the `testsuite` repo we can pull in the official tests.
3720508 to
4aea7b7
Compare
aheejin
left a comment
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.
LGTM % few nits
ac77ddf to
d5f65a4
Compare
d5f65a4 to
c99744e
Compare
|
This does sort of raise the question of how a tool should handle compact imports. Since it looks like there will be both a text and a binary format, you could argue that a true "pass-through/roundtrip" encoding would only encode compact imports in binary where they are encoded in the text. But of course it also makes sense to encode them everywhere possible, just to reduce binary size. Maybe wabt should default to only round-tripping? (And optionally have a flag to opportunistically use compact imports?) |
Right now that flag is I guess if we might have users who want to enable them in the reader but disabled them in the writer, but so far that hasn't come up for other features. |
|
Right, for Binaryen, I don't know of any case where we wouldn't want to opportunistically use compact imports when they are enabled. Wabt is more interesting, because its job is to be a low-level tool that round-trips text and binary, rather than an optimization/transformation tool, and changing the encoding is a transformation. |
|
Yeah, right no the compact imports are not remembered on read, and we write them purely opportunistically. We should probably followup by preserving them in the IR instead. |
See https://github.com/WebAssembly/compact-import-section
For this initial commit I've just added a single simple test in form of test/binary/compact-imports.txt. Once we update the
testsuiterepo we can pull in the official tests.