-
Notifications
You must be signed in to change notification settings - Fork 831
Type check block/loop/if sigs #717
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
|
This also
|
dschuff
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.
Do we actually still need to do all the type merging now? Seems like if we give blocks their type on construction then we don't need to do anything on finalizing, and we can check break types, etc without that?
src/wasm-binary.h
Outdated
| } | ||
| // For blocks with type unreachable but whose breaks have arity 1, encode i32 as their | ||
| // signature so that the decoder knows to pop a value for the breaks' values. | ||
| o << binaryWasmType(curr->type != unreachable ? curr->type : arity ? i32 : none); |
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.
Once we parse block signatures, we can just encode curr->type here which should never be unreachable. Likewise for ifs and loops.
|
Finalizing might still matter for code generators other than wasm-s-parser. But maybe that is worth refactoring later into something on the side. |
|
So the binary format difference is because in the wast parser you replaced the block finalization with the logic that instead only marks the block as unreachable if no breaks target it and the fallthrough is unreachable. If you do that in the binary parser too then it works correctly (I have that working in my local branch). I do think the "primary" behavior should be that the type of blocks doesn't get changed by default. It might make sense to have a service for binaryen library users that might not know the block type up front (which I guess may include the optimization passes) but it doesn't need to have the same behavior. |
|
Yeah, as discussed in person that's indeed the issue. I'll continue work towards that, with |
…ats. print if and loop sigs which were missing. remove dsl from OptimizeInstructions as after those changes it needs rethinking
a48b4b8 to
c6fe3db
Compare
|
Ok, this is hopefully good to go. Tests pass locally. |
This rewrites the type checking code to use block/if/loop sigs properly, as read in s-expr files.
This passes all the spec tests and is basically done I think. but as mentioned in #711 (comment) fails on the binary format tests on round-tripping.
This is based on #711, so it should be rebased on master after that lands.