-
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
Changes from 1 commit
f9ab9d4
2a932d3
e96014a
6b8787d
a3cc291
dfbb193
ef0a683
53e40d4
8f85434
b475268
3c1906e
6ab20ad
3556171
bc86090
97d54f7
cf17036
db5b02a
115d0e2
143bb2d
6749e08
17be964
c262d3c
fbcb7fb
06a26c1
d41256d
ab6ff8b
179faa4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -958,6 +958,20 @@ class EffectAnalyzer { | |
// traps when ref is null. | ||
parent.implicitTrap = true; | ||
} | ||
|
||
void visitResume(Resume* curr) { | ||
// This acts as a kitchen sink effect. | ||
parent.calls = true; | ||
|
||
// FIXME(frank-emrich) We should probably set parent.trap or | ||
// parent.implicitTrap, because the resume instruction itself may trap: | ||
// The continuation reference is nullable, and we trap if it is indeed | ||
// null. | ||
frank-emrich marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. This was just inspired by the following happening at the end of
I'm not sure if it's really necessary for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if the resumed continuation throws, it will bubble out from the |
||
parent.throws_ = true; | ||
} | ||
} | ||
}; | ||
|
||
public: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -889,6 +889,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 commentThe reason will be displayed to describe this comment to others. Learn more. It seems we need to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
DELEGATE_FIELD_CHILD(Resume, cont); | ||
DELEGATE_FIELD_CHILD_VECTOR(Resume, operands); | ||
DELEGATE_FIELD_SCOPE_NAME_USE_VECTOR(Resume, handlerBlocks); | ||
DELEGATE_FIELD_NAME_KIND_VECTOR(Resume, handlerTags, ModuleItemKind::Tag); | ||
DELEGATE_FIELD_HEAPTYPE(Resume, contType); | ||
DELEGATE_END(Resume); | ||
break; | ||
} | ||
} | ||
|
||
#undef DELEGATE_ID | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2342,6 +2342,7 @@ class ConstantExpressionRunner : public ExpressionRunner<SubType> { | |
} | ||
return ExpressionRunner<SubType>::visitRefAs(curr); | ||
} | ||
Flow visitResume(Resume* curr) { return Flow(NONCONSTANT_FLOW); } | ||
|
||
void trap(const char* why) override { throw NonconstantException(); } | ||
|
||
|
@@ -3857,6 +3858,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 commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
void trap(const char* why) override { externalInterface->trap(why); } | ||
|
||
|
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 thatvalidateBinaryenIR
eventually re-finalises each block by callingBlock::finalize()
. This can cause situations where the types collected byBranchSeeker
fully determine the new type given to the block, ignoring the type annotation on it. This means that the code snippet here, whereType::none
is passed as a placeholder for the real type causes problems, becauseBlock::finalize()
will obtain this fromBranchSeeker
and uses it as the overall type of the block. This then makesvalidateBinaryenIR
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 itshas
andcount
functions. These don't care about the types sent to the branches types at all. In fact, there are few uses ofBranchSeeker
that do inspect thetypes
field afterwards. And we only need access to theModule
in order to correctly calculate the types sent to the branch. That means we would have to changeBranchseeker::count
andBranchSeeker::has
to take aModule
parameter, even though thetypes
field is ignored.This suggests that it may be useful to change the existing
BranchSeeker
such that it continues offeringhas
andcount
with the existing signatures but remove thetypes
field. This could be implemented entirely usingoperateOnScopeNameUses
, no type (or module) information needed.We then define a new
BranchTypeSeeker
that behaves more like the oldBranchSeeker
, which usesoperateOnScopeNameUsesAndSentTypes
, does accumulate the types sent to the branch, and needs access to theModule
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 ofBlock
: The existingfinalize()
must definitely take aModule
in the future, because it usesBranchSeeker
to actually inspect the types sent to branches.However,
Block::finalize(Type type_)
andBlock::finalize(Type type_, Breakability breakability)
only useBranchUtils::BranchSeeker::has
and don't care about the types sent to the block. Thus, if we go through with the splitting ofBranchSeeker
mentioned above, we could avoid also needing to add aModule
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 ofBranchseeker::count
andBranchSeeker::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 theReFinalize
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 theModule
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 calledsentTypes
.There is one different issue here that affects the overall implementation of
operateOnScopeNameUsesAndSentTypes
: Since it is implemented usingoperateOnScopeNameUses
, aResume
instruction with n handler clauses results in n calls to the lambda defined inoperateOnScopeNameUsesAndSentTypes
, with the name of the corresponding handler block as thename
argument each time. However, in the lambda we then have would run code like this: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.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.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
inoperateOnScopeNameUsesAndSentValues
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
? Withtry_table
, at least LLVM at the moment will generate instructions with only one clause (it is either for the C++ tag/label or thecatch_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 aVisitor
.@aheejin There are some compilation schemes where you may create
resume
instructions withtag
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.