-
Notifications
You must be signed in to change notification settings - Fork 747
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
Typed continuations: resume instructions #6083
Conversation
There are currently a few |
src/ir/branch-utils.h
Outdated
// We could also store this information in the Resume node itself, but | ||
// that would mean storing the signature of all the Tags that we have a | ||
// handle clause for. | ||
func(name, Type::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.
The issue described here is what's causing the lit test suite to fail:
Enabling BINARYEN_PASS_DEBUG
means that validateBinaryenIR
eventually re-finalises each block by calling Block::finalize()
. This can cause situations where the types collected by BranchSeeker
fully determine the new type given to the block, ignoring the type annotation on it. This means that the code snippet here, where Type::none
is passed as a placeholder for the real type causes problems, because Block::finalize()
will obtain this from BranchSeeker
and uses it as the overall type of the block. This then makes validateBinaryenIR
fail.
This failure seems to be a relatively recent change, since the test suite was passing a few weeks ago when I did the majority of the work on this PR... But I guess it just shows that my "solution" of passing Type::none
here was never quite right to begin with.
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.
cc @aheejin, I guess the new EH design will also run into the same problem here: the types of values carried on the branches from the catch_table
instruction will depend on the tags on that instruction, so we will need the module here to look up the types of those tags.
@frank-emrich, I don't think we have a choice but to plumb the module through to here sooner or later, and probably sooner, since I can't think of a good workaround 🤔
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.
I've worked on this for a bit, which mostly involved changing all use sites of BranchSeeker
, and have a question:
Currently, BranchSeeker
is mostly used by calling its has
and count
functions. These don't care about the types sent to the branches types at all. In fact, there are few uses of BranchSeeker
that do inspect the types
field afterwards. And we only need access to the Module
in order to correctly calculate the types sent to the branch. That means we would have to change Branchseeker::count
and BranchSeeker::has
to take a Module
parameter, even though the types
field is ignored.
This suggests that it may be useful to change the existing BranchSeeker
such that it continues offering has
and count
with the existing signatures but remove the types
field. This could be implemented entirely using operateOnScopeNameUses
, no type (or module) information needed.
We then define a new BranchTypeSeeker
that behaves more like the old BranchSeeker
, which uses operateOnScopeNameUsesAndSentTypes
, does accumulate the types sent to the branch, and needs access to the Module
to do so to implement typed continuations and the new style EH.
This question came up when changing the signature of the various finalize
methods of Block
: The existing finalize()
must definitely take a Module
in the future, because it uses BranchSeeker
to actually inspect the types sent to branches.
However, Block::finalize(Type type_)
andBlock::finalize(Type type_, Breakability breakability)
only use BranchUtils::BranchSeeker::has
and don't care about the types sent to the block. Thus, if we go through with the splitting of BranchSeeker
mentioned above, we could avoid also needing to add a Module
parameter to these two.
I've mostly gone through with changing the signatures of all three Block::finalize
functions now, so I don't care much which option we take here, but it felt a bit unfortunate to change all these use sites of Branchseeker::count
and BranchSeeker::has
, even though they don't actually care about the extra typing information.
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.
This refactoring sounds good to me.
One option might be for the finalize
methods that would need to take a Module to instead conservatively keep the type unchanged, then have special logic in the ReFinalize
that takes the module into account to do better refinalization.
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.
I have created a separate PR with the Module parameter plumbing: #6096. I will rebase this PR once the other one has landed.
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.
After the discussion in #6096 I will now implement storing the sent types directly in Resume
nodes, instead of using the Module
here to determine them.
The most straightforward implementation is an ArenaVector<Type>
whose i-th entry contains the type sent to the i-th block (i.e, curr->handlerBlocks[i]
). Let's assume this vector is called sentTypes
.
There is one different issue here that affects the overall implementation of operateOnScopeNameUsesAndSentTypes
: Since it is implemented using operateOnScopeNameUses
, a Resume
instruction with n handler clauses results in n calls to the lambda defined in operateOnScopeNameUsesAndSentTypes
, with the name of the corresponding handler block as the name
argument each time. However, in the lambda we then have would run code like this:
...
} else if (auto* res = expr->dynCast<Resume>()) {
for (usize i = 0; < res->handlerBlocks.size(); i++) {
auto handlerBlock = res->handlerBlocks[i];
if (handlerBlock == name) {
func(name, curr->sentTypes[i]);
}
}
} else { ...
In total, this is quadratic in the number of handler clauses.
Which of the following solutions would you prefer:
operateOnScopeNameUses
takes a lambda whose parameter is avector
of names. We would effectively call the lambda "in bulk" for those node types that have more than one scope name use. But this would require creating singleton vector for all the other node types, so this doesn't seem efficient.- We could pass some kind of index to the lambda, indicating that the currently passed
Name
is the i-th block name used by the particular node. But that doesn't seem great either, the index would be meaningless for all node types other thanResume
. sentTypes
could be a map from block names to types instead of just a vector of types. That seems relatively clean, but uses more space, and it would need to be integrated into the logic that updates tag names in case a naming section is encountered in a binary.- We may implement
operateOnScopeNameUsesAndSentValues
without usingoperateOnScopeNameUses
at all: Instead, it could just be a visitor on Expressions. In the case forResume
nodes, we would then process all the blocks at once and invokefunc
for each handler block.
I'm personally in favor of the last option: After all, the lambda passed to operateOnScopeNameUses
in operateOnScopeNameUsesAndSentValues
already disambiguates the different node types, operateOnScopeNameUses
is basically just used to pre-filter the nodes and extract the name of the used block.
Note that this entire discussion equally applies to Try
nodes once the updated EH proposal is implemented.
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.
4 makes sense to me as well.
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.
How many handler clauses do you expect to have with resume
? With try_table
, at least LLVM at the moment will generate instructions with only one clause (it is either for the C++ tag/label or the catch_all
label. We don't generate both at the same time. But even if we do it's gonna be only two), so we are not really gonna be affected by the quadratic loop. If what you would have in average is less than 5, I'm not sure if this is something we should worry about?
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.
I've now re-implemented operateOnScopeNameUsesAndSentTypes
using a Visitor
.
@aheejin There are some compilation schemes where you may create resume
instructions with tag
clauses that handle more or less every single tag defined. However, even then I'd surprised to see more than a dozen or two. However, given that there are source languages with effect handlers that would like to target our instructions directly, it's basically up to what people do in those source languages. Having the quadratic behavior here is probably fine unless someone translates some crazy generated code using effect handlers into wasm.
But given that the solution using a visitor was relatively simple to implement, I'd say let's just err on the side of caution.
src/ir/branch-utils.h
Outdated
// We could also store this information in the Resume node itself, but | ||
// that would mean storing the signature of all the Tags that we have a | ||
// handle clause for. | ||
func(name, Type::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.
This refactoring sounds good to me.
One option might be for the finalize
methods that would need to take a Module to instead conservatively keep the type unchanged, then have special logic in the ReFinalize
that takes the module into account to do better refinalization.
I've now addressed the first round of comments. One thing that I've noticed is that I've had a quick look at the logic in |
I think the |
That doesn't seem to have worked if I just move it on its own line in the input function. Did you mean changing the printer such that it goes on its own line in the output? That seems tricky, because there may be many such clauses, so at some point they have to go on their own lines, rather than unconditionally keeping them all on the same line as the |
Yes, I think you'd have to change the printer to put the |
I've changed the printing now so that all the In general, things should be ready now for you to have a look at. |
This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in WebAssembly#6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in WebAssembly#6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to WebAssembly#6083 (comment) and WebAssembly#6096 for related discussions when `resume` was added.
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.
Generally looks good, just a few final comments.
src/ir/branch-utils.h
Outdated
func(name, br->getSentType()); | ||
} else { | ||
assert(expr->is<Try>() || expr->is<Rethrow>()); // delegate or rethrow | ||
struct SentTypesVisitor : public Visitor<SentTypesVisitor> { |
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.
Just in the interest of minimizing the change, can this go back to being an if-else chain?
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.
Done
@@ -3913,6 +3914,7 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> { | |||
multiValues.pop_back(); | |||
return ret; | |||
} | |||
Flow visitResume(Resume* curr) { return Flow(NONCONSTANT_FLOW); } |
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.
This should probably be WASM_UNREACHABLE("unimplemented")
. NONCONSTANT_FLOW
is only used in the ExpressionRunner
up above.
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.
Done
// on null. | ||
parent.implicitTrap = true; | ||
|
||
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { |
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.
Is this because we're modeling suspending as throwing? Probably worth a comment if so.
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.
This was just inspired by the following happening at the end of visitCall
here :
// When EH is enabled, any call can throw.
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {
parent.throws_ = true;
}
I'm not sure if it's really necessary for Resume
. @aheejin would you mind having a look?
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.
Can a resume instruction throw an exception? If not, we can remove this. (Calls can, which is why they have it.)
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.
Yes, if the resumed continuation throws, it will bubble out from the resume
instruction, just like an exception from a called function would.
@@ -331,6 +331,8 @@ struct SubtypingDiscoverer : public OverriddenVisitor<SubType> { | |||
void visitStringIterMove(StringIterMove* curr) {} | |||
void visitStringSliceWTF(StringSliceWTF* curr) {} | |||
void visitStringSliceIter(StringSliceIter* curr) {} | |||
|
|||
void visitResume(Resume* curr) { WASM_UNREACHABLE("not implemented"); } |
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.
This should just note that the body is a subtype of the resume's type.
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.
I think It's slightly more complicated than that: We may also need to report subtyping relations induced by values being sent to handler blocks: If a resume
instruction has a (tag $t $h)
clause, then the n param types of $t
must match the first n result types of block $h
. I guess this means that we would have to record these relationships here and skipped it so far.
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.
Looking good; mostly nits. I appreciate your discussions on the tag types in #6083 (comment) and #6096, which I referred to before I submitted #6181.
src/passes/Print.cpp
Outdated
o << ' '; | ||
printHeapType(curr->contType); | ||
|
||
// We deliberate keep all (tag ...) clauses on the same line as the resume | ||
// itself to work around a quirk in update_lit_checks.py | ||
for (size_t i = 0; i < curr->handlerTags.size(); i++) { | ||
o << maybeSpace << '('; | ||
printMedium(o, "tag "); | ||
printName(curr->handlerTags[i], o); | ||
o << " "; | ||
printName(curr->handlerBlocks[i], o); | ||
o << ")"; | ||
} |
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.
+1 on moving this to PrintExpressionContents::visitResume
. PrintSExpression
seems to be mainly for child expressions.
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.
Done
@@ -899,6 +899,17 @@ switch (DELEGATE_ID) { | |||
DELEGATE_END(StringSliceIter); | |||
break; | |||
} | |||
|
|||
case Expression::Id::ResumeId: { | |||
DELEGATE_START(Resume); |
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.
It seems we need to add sentTypes
here as well. See #6181 (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.
Done
src/wasm/wasm.cpp
Outdated
ArenaVector<Type>& Resume::getSentTypes() { | ||
if (this->sentTypes.size() != this->handlerBlocks.size()) { | ||
Fatal() << "Types sent to blocks have not been determined, yet."; | ||
} | ||
return this->sentTypes; | ||
} |
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 need to make sentTypes
a private member and have this function separately? I think we can access it directly, and then this if
check might better be in the validator.
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.
Done
src/wasm/wasm-binary.cpp
Outdated
auto resume = allocator.alloc<Resume>(); | ||
curr = resume; | ||
visitResume(resume); |
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.
auto resume = allocator.alloc<Resume>(); | |
curr = resume; | |
visitResume(resume); | |
visitResume((curr = allocator.alloc<Resume>())->cast<Resume>()); |
I'm not sure why MemorySize
and MemoryGrow
above didn't use it, but can we do this one-liner we used for other instructions above?
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.
Done
@@ -0,0 +1,94 @@ | |||
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. |
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.
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.
Done
src/wasm.h
Outdated
void finalize(); | ||
void finalize(Module* wasm); |
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.
Can we
void finalize(); | |
void finalize(Module* wasm); | |
void finalize(Module* wasm = nullptr); |
Would this be simpler?
Also it would be nice if we have some comments, as suggested with TryTable
in https://github.com/WebAssembly/binaryen/pull/6181/files#r1426054122.
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.
Done
src/wasm/wasm.cpp
Outdated
void Resume::finalize() { | ||
// Only performing some validation, but no finalization. The finalize(Module*) | ||
// function must have been called previously. | ||
|
||
if (!(this->contType.isContinuation() && | ||
this->contType.getContinuation().type.isSignature())) { | ||
Fatal() << "ill-formed Resume expression"; | ||
} | ||
|
||
if (this->sentTypes.size() != this->handlerBlocks.size()) { | ||
Fatal() << "Resume node was not finalized"; | ||
} | ||
} | ||
|
||
void Resume::finalize(Module* wasm) { | ||
if (!(this->contType.isContinuation() && | ||
this->contType.getContinuation().type.isSignature())) { | ||
Fatal() << "ill-formed Resume expression"; | ||
} |
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.
Can we move this (validation in Resume::finalize()
and Resume::finalize(Module* wasm)
to wasm-validator.cpp
? Also if we do
void finalize(Module* wasm = nullptr);
in wasm.h
, we wouldn't need two methods.
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.
Done
;; CHECK-TEXT: (type $3 (func (param (ref $ct)) (result i32))) | ||
|
||
;; CHECK-TEXT: (tag $t (result i32)) | ||
(tag $t (result i32)) |
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.
Can we have a test with resume
using a tag with non-empty params
as well?
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.
Done, I've just changed $t
in this test.
src/wasm/wasm.cpp
Outdated
if (tgps.size() > 0) { | ||
TypeList sentValueTypes; | ||
sentValueTypes.reserve(tgps.size() + 1); | ||
|
||
sentValueTypes.insert(sentValueTypes.begin(), tgps.begin(), tgps.end()); | ||
sentValueTypes.push_back(ctPrimeRef); | ||
this->sentTypes[i] = Type(sentValueTypes); | ||
} else { | ||
this->sentTypes[i] = ctPrimeRef; | ||
} |
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 need these separate cases? The body within if
would work fine even if tgps
is empty.
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.
Good point! The else
branch is probably the more common case, where the handler block only receives the continuation, but no additional payloads. So separating the two cases just boils down to avoiding the allocation for a singleton std::vector<Type>
(namely, sentValueTypes
). Would you prefer avoiding the separate else
case?
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.
The current code looks good to me as is.
This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in #6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in #6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to #6083 (comment) and #6096 for related discussions when `resume` was added.
- sentTypes is public now - only one finalize method with optional Module* parameter - populateResumeSentTypes helper function
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.
Thanks!
shouldBeTrue( | ||
curr->sentTypes.size() == curr->handlerBlocks.size(), | ||
curr, | ||
"sentTypes cache in Resume instruction has not been initialised"); |
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.
"sentTypes cache in Resume instruction has not been initialised"); | |
"sentTypes cache in Resume instruction has not been initialized"); |
Nit: Binaryen codebase seems to use American spelling, so...
src/wasm/wasm.cpp
Outdated
if (tgps.size() > 0) { | ||
TypeList sentValueTypes; | ||
sentValueTypes.reserve(tgps.size() + 1); | ||
|
||
sentValueTypes.insert(sentValueTypes.begin(), tgps.begin(), tgps.end()); | ||
sentValueTypes.push_back(ctPrimeRef); | ||
this->sentTypes[i] = Type(sentValueTypes); | ||
} else { | ||
this->sentTypes[i] = ctPrimeRef; | ||
} |
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.
The current code looks good to me as is.
Oops, I saw your spelling nit just after I merged this, @aheejin. I'll fix it. |
This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in WebAssembly#6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in WebAssembly#6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to WebAssembly#6083 (comment) and WebAssembly#6096 for related discussions when `resume` was added.
This PR is part of a series that adds basic support for the [typed continuations proposal](https://github.com/wasmfx/specfx). This particular PR adds support for the `resume` instruction. The most notable missing feature is validation, which is not implemented, yet.
This PR is part of a series that adds basic support for the typed continuations proposal.
This particular PR adds support for the
resume
instruction. The most notable missing feature is validation, which is not implemented, yet.