Stack switching proposal support #7041
Conversation
dhil
force-pushed
the
stack-switching
branch
2 times, most recently
from
1be4ea5
to
d531797
Compare
This patch implements text and binary encoding/decoding support for the stack switching proposal. It does so by adapting the previous typed-continunations implementation. Particular changes: * Support for new `resume` encoding. * Added support for `resume_throw` and `switch`. * Feature flag `typed-continuations` has been renamed to `stack-switching`. A small unfortunate implementation detail is that the internal name `Switch` was already taken by the `br_table` instruction, so I opted to give the `switch` instruction the internal name `StackSwitch`. A minor detail is that I have reordered the declarations/definitions of the stack switching instructions such that they appear in ascending order according to their opcode value (this is the same order that the stack-switching explainer document present them in).
|
Also, if you could avoid force pushes as you update the PR, that will help me see what changes from version to version. |
Yes, I only did that while rebasing and noone were reviewing :-) I will implement your feedback and push it directly on top of here. |
'ContNew' and 'ContBind'
|
@tlively I think I have addressed all of your feedback now. Please take a look when you got time. Thanks in advance! |
| @@ -301,7 +301,7 @@ struct PrintSExpression : public UnifiedExpressionVisitor<PrintSExpression> { | |||
| void visitLoop(Loop* curr); | |||
| void visitTry(Try* curr); | |||
| void visitTryTable(TryTable* curr); | |||
| void visitResume(Resume* curr); | |||
There was a problem hiding this comment.
For the expressions that now depend on the types of their children to recover the printed type annotations, we now need to handle the case where the children are unreachable or have bottom heap types. See what we do for StructNew and friends just below this. It would also be good to add tests that exercise these cases.
This reverts commit d1866bc.
|
Do you prefer I resolve the merge conflict via rebase+force-push or just as a merge commit? |
A merge would be good, thanks! |
src/wasm/wasm-binary.cpp
Outdated
| HeapType ftPrime{Signature(tgrs, ctrs)}; | ||
| HeapType ctPrime{Continuation(ftPrime)}; | ||
| Type ctPrimeRef(ctPrime, Nullability::NonNullable); |
There was a problem hiding this comment.
Shouldn't this be looking up the types based on the labels now?
There was a problem hiding this comment.
Is your point that we can simply read the type of the continuation off the (corresponding branch target) label type? I think so yes, it must have been given statically in the source code. It is not entirely clear to me what the purpose of sentTypes is (I suppose it is some caching mechanism).
src/wasm/wasm-ir-builder.cpp
Outdated
| HeapType ftPrime{Signature(tgrs, ctrs)}; | ||
| HeapType ctPrime{Continuation(ftPrime)}; | ||
| Type ctPrimeRef(ctPrime, Nullability::NonNullable); |
This patch implements text and binary encoding/decoding support for the stack switching proposal. It does so by adapting the previous typed-continunations implementation. Particular changes:
resumeencoding.resume_throwandswitch.typed-continuationshas been renamed tostack-switching.A small unfortunate implementation detail is that the internal name
Switchwas already taken by thebr_tableinstruction, so I opted to give theswitchinstruction the internal nameStackSwitch.A minor detail is that I have reordered the declarations/definitions of the stack switching instructions such that they appear in ascending order according to their opcode value (this is the same order that the stack-switching explainer document present them in).
I can look into adding validation support in a subsequent patch.