Unify method pairs with and without Type param #6184
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As suggested in WebAssembly#6181 (comment), using `std::optional<Type>`, this unifies two different versions of `make***`, for block-like structures (`block`, `if`, `loop`, `try`, and `try_table`) with and without a type parameter. This also allows unifying of `finalize` methods, with and without a type. This also sets `breakability` argument of `Block::finalize` to `Unknown` so we can only have one `Block::finalize` that handles all cases. This also adds an optional `std::optional<Type> type` parameter to `blockify`, `blockifyWithName`, and `makeSequence` functions in `wasm-builder.h`.
aheejin
changed the title
kripken
approved these changes
Dec 19, 2023
| type = *type_; | ||
| if (type == Type::none && list.size() > 0) { | ||
| handleUnreachable(this, breakability); | ||
| } |
There was a problem hiding this comment.
Maybe early-return after this to avoid indenting all the rest in an else?
There was a problem hiding this comment.
Done. Also did the same for If::finalize. (Others (Loop, Try, and TryTable) were kinda small and didn't seem worth it)
tlively
approved these changes
Dec 19, 2023
src/wasm-builder.h
Outdated
Comment on lines
1288
to
1290
| Block* blockify(Expression* any, | ||
| Expression* append = nullptr, | ||
| std::optional<Type> type = std::nullopt) { |
There was a problem hiding this comment.
Oh neat, does this mean we can pass a type to the variadic version of blockify below now, too?
There was a problem hiding this comment.
I thought so, but it doesn't seem to work... It continues to blockify(any, append) and errors out as soon as the append is not Expression*, like this:
/usr/local/google/home/aheejin/binaryen/src/wasm-builder.h:1307:12: error: no matc
hing member function for call to 'blockify'
1307 | return blockify(blockify(any, append), args...);
| ^~~~~~~~
/usr/local/google/home/aheejin/binaryen/src/passes/StackCheck.cpp:112:25: note: in
instantiation of function template specialization 'wasm::Builder::blockify<wasm::
Type>' requested here
112 | auto *ret = builder.blockify(check, newSet, Type(Type::i32));
| ^
/usr/local/google/home/aheejin/binaryen/src/wasm-builder.h:1288:10: note: candidat
e function not viable: no known conversion from 'wasm::Type' to 'Expression *' for
2nd argument
1288 | Block* blockify(Expression* any,
| ^
1289 | Expression* append = nullptr,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/aheejin/binaryen/src/wasm-builder.h:1306:10: note: candidat
e function template not viable: no known conversion from 'wasm::Type' to 'Expressi
on *' for 2nd argument
1306 | Block* blockify(Expression* any, Expression* append, Ts... args) {
| ^ ~~~~~~~~~~~~~~~~~~
1 error generated.I guess I should revert the blockify part. blockifyWithName should be fine given that it doesn't use a variadic parameter though.
src/wasm/wasm.cpp
Outdated
Comment on lines
201
to
204
| // Calculate the supertype of the branch types and the flowed-out type. If | ||
| // there is no supertype among the available types, assume the current | ||
| // type is already correct. TODO: calculate proper LUBs to compute a new | ||
| // correct type in this situation. |
There was a problem hiding this comment.
This comment looks extremely stale since we do calculate LUBs now.
src/wasm/wasm.cpp
Outdated
Comment on lines
884
to
885
| // If none of the component bodies' type is a supertype of the others, | ||
| // assume the current type is already correct. TODO: Calculate a proper LUB. |
tlively
approved these changes
Dec 21, 2023
I ran |
radekdoulik
pushed a commit
to dotnet/binaryen
that referenced
this pull request
Jul 12, 2024
As suggested in WebAssembly#6181 (comment), using `std::optional<Type>`, this unifies two different versions of `make***`, for block-like structures (`block`, `if`, `loop`, `try`, and `try_table`) with and without a type parameter. This also allows unifying of `finalize` methods, with and without a type. This also sets `breakability` argument of `Block::finalize` to `Unknown` so we can only have one `Block::finalize` that handles all cases. This also adds an optional `std::optional<Type> type` parameter to `blockifyWithName`, and `makeSequence` functions in `wasm-builder.h`. blockify was not included because it has a variadic parameter.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As suggested in
#6181 (comment), using
std::optional<Type>, this unifies two different versions ofmake***, for block-like structures (block,if,loop,try, andtry_table) with and without a type parameter.This also allows unifying of
finalizemethods, with and without a type. This also setsbreakabilityargument ofBlock::finalizetoUnknownso we can only have oneBlock::finalizethat handles all cases.This also adds an optional
std::optional<Type> typeparameter toblockifyWithName, andmakeSequencefunctions inwasm-builder.h.blockifywas not included because it has a variadic parameter.