-
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: cont.new instructions #6308
Conversation
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.
Looks great! Just a few small comments.
src/ir/cost.h
Outdated
@@ -726,6 +726,10 @@ struct CostAnalyzer : public OverriddenVisitor<CostAnalyzer, CostType> { | |||
return 8 + visit(curr->ref) + visit(curr->num); | |||
} | |||
|
|||
CostType visitContNew(ContNew* curr) { | |||
// Some arbitrary "high" value, reflecting that this may allocate a stack | |||
return 10 + visit(curr->func); |
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.
Would you expect that this would be cheaper than resume
, which we model with a base cost of 12?
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.
Ha, good point! If the stack allocation does happen at cont.new
, this should indeed be more expensive than resume
. But I could also imagine an implementation strategy where the allocation happens lazily when resume
-ing a new continuation for the first time.
In general, I have little confidence in the numbers here. For now, I've increased the base cost for cont.new
to 14, which is still very arbitrary, but at least greater than the value for resume
. Should I add a TODO that we should probably revisit these values at some point? (e.g., once we have more experience from implementing the extension)
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.
Ehh, probably no need for a TODO. All of the numbers in this file are similarly suspect.
src/wasm/wasm-ir-builder.cpp
Outdated
Result<> IRBuilder::visitContNew(ContNew* curr) { | ||
auto func = pop(); | ||
CHECK_ERR(func); | ||
curr->func = *func; | ||
|
||
return Ok{}; | ||
} |
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 default visitExpression
implementation will already do exactly this, so I don't think we need to implement visitConstNew
here at all. If what we popped depended on the contType
at all, that would be different.
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.
oh, nice!
Co-authored-by: Thomas Lively <tlively123@gmail.com>
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, thanks!
This PR is part of a series that adds basic support for the [typed continuations/wasmfx proposal](https://github.com/wasmfx/specfx). This particular PR adds support for the `cont.new` instruction for creating continuations, documented [here(https://github.com/wasmfx/specfx/blob/main/proposals/continuations/Overview.md#instructions). In short, these instructions are of the form `(cont.new $ct)` where `$ct` must be a continuation type. The instruction takes a single (nullable) function reference as its argument, which means that the folded representation of the instruction is of the form `(cont.new $ct (foo ...))`. Support for the instruction is implemented in both the old and the new wat parser. Note that this PR does not implement validation of the new instruction.
This PR is part of a series that adds basic support for the [typed continuations/wasmfx proposal](https://github.com/wasmfx/specfx). This particular PR adds support for the `cont.new` instruction for creating continuations, documented [here(https://github.com/wasmfx/specfx/blob/main/proposals/continuations/Overview.md#instructions). In short, these instructions are of the form `(cont.new $ct)` where `$ct` must be a continuation type. The instruction takes a single (nullable) function reference as its argument, which means that the folded representation of the instruction is of the form `(cont.new $ct (foo ...))`. Support for the instruction is implemented in both the old and the new wat parser. Note that this PR does not implement validation of the new instruction.
This PR is part of a series that adds basic support for the typed continuations/wasmfx proposal.
This particular PR adds support for the
cont.new
instruction for creating continuations, documented here.In short, these instructions are of the form
(cont.new $ct)
where$ct
must be a continuation type. The instruction takes a single (nullable) function reference as its argument, which means that the folded representation of the instruction is of the form(cont.new $ct (foo ...))
.Support for the instruction is implemented in both the old and the new wat parser.
Note that this PR does not implement validation of the new instruction.