Stack switching proposal support #7041

dhil · 2024-10-30T14:59:40Z

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).

I can look into adding validation support in a subsequent patch.

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).

tlively

Thanks for working on this! Happy to meet and discuss any of these comments in real time if it would be more efficient for you.

src/wasm.h

src/wasm/wasm.cpp

src/parser/parsers.h

src/wasm/wasm-ir-builder.cpp

src/wasm/wasm-validator.cpp

tlively · 2024-11-05T02:15:52Z

Also, if you could avoid force pushes as you update the PR, that will help me see what changes from version to version.

dhil · 2024-11-05T09:07:26Z

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'

dhil · 2024-11-06T17:28:14Z

@tlively I think I have addressed all of your feedback now. Please take a look when you got time. Thanks in advance!

tlively

Thanks for making the changes! Here are a few more comments, but I think we're getting close.

src/parser/parsers.h

tlively · 2024-11-06T18:01:34Z

src/passes/Print.cpp

@@ -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);


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.

Please take a look at commits 157552a and 480843d.

src/wasm/wasm-validator.cpp

test/lit/basic/stack_switching_resume_throw.wast

src/wasm/wasm-binary.cpp

This reverts commit d1866bc.

src/passes/Print.cpp

src/wasm/wasm.cpp

dhil · 2024-11-18T10:55:31Z

Do you prefer I resolve the merge conflict via rebase+force-push or just as a merge commit?

tlively · 2024-11-18T17:34:50Z

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

src/wasm/wasm-ir-builder.cpp

This commit is a partial solution to removing the type immediate members from `cont.new,cont.bind,resume,resume_throw,switch`. However, when I fully remove the type immediates then I observe a crash in `child-ir.h` on `visitContBind,visitResume,visitResumeThrow,visitStackSwitch`. It seems that `curr->cont->type` is sometimes not a continuation type...

dhil · 2024-11-28T12:14:10Z

@tlively I am experiencing some difficulties with getting rid of the type immediate members (contType). Whenever I swap contType for curr->cont->type.getHeapType() in child-typer.h I observe crashes in the ConstraintCollector. Supposedly, curr->cont->type is not always a continuation type... so something is wrong. I need another set of eyes to have a look. When you got time, do you mind taking a look at the code?

tlively · 2024-12-02T16:57:10Z

Check out the pattern used by other instructions that take type immediates in child-typer.h, for example visitStructGet and visitStructSet. They take an optional additional argument for the type, and if that argument is present, it is used instead of looking up the type from the instruction operand. IRBuilder always passes these type arguments through to ChildTyper.

dhil · 2024-12-03T10:27:48Z

Thanks for the pointer. I have sorted it out now, I think. The latest commit ought to eliminate the type immediates from the stack switching instructions.

tlively

Looks great! Just a few comments on the tests.

tlively · 2024-12-03T16:38:41Z

test/lit/basic/stack_switching_contnew.wast

@@ -57,13 +77,38 @@
   (cont.new $ct (ref.func $g))
 )

+ (func (result (ref cont))


If you give this function a $name, its test output will appear next to it correctly.

tlively · 2024-12-03T16:54:28Z

test/lit/basic/stack_switching_resume_throw.wast

It would be good to test unreachable continuation operands here and for switch as well.

tlively · 2024-12-03T17:59:11Z

test/lit/basic/stack_switching.wast

+    (suspend $e1)
+  )
+
+  (func (export "unhandled-2")


The output will be more readable if all these functions have $names as well.

dhil force-pushed the stack-switching branch 2 times, most recently from 1be4ea5 to d531797 Compare November 1, 2024 15:43

dhil force-pushed the stack-switching branch from d531797 to 14af574 Compare November 4, 2024 16:20

tlively reviewed Nov 5, 2024

View reviewed changes

dhil added 9 commits November 5, 2024 11:11

Address the 'easy' bits of Thomas' feedback

edbc438

Deduplicate the parsing logic for resume tables

c0d8648

Use inherited the 'type' member to track the continuation type in

98f6314

'ContNew' and 'ContBind'

Format

c407262

Remove PrintSExpression::visit{Resume,ResumeThrow,StackSwitch}

15fcbc6

Don't (re)compute sentTypes for resume and resume_throw.

d1866bc

[fuzz] Skip stack switching tests.

2b06c76

Abstract shared logic for reading resume tables.

3b339f9

Encode on-clauses using only two parallel vectors.

0886451

tlively reviewed Nov 6, 2024

View reviewed changes

dhil added 6 commits November 7, 2024 10:42

Return resumetable rather than taking it as a reference.

d76f260

Revert "Don't (re)compute sentTypes for resume and resume_throw."

1c2ed8a

This reverts commit d1866bc.

Propagate Type::unreachable in switch.

c8bf255

Compute sentTypes for resumetables upon construction.

9e16e47

WIP

157552a

Check for unreachable continuation types.

480843d

tlively reviewed Nov 12, 2024

View reviewed changes

src/passes/Print.cpp Outdated Show resolved Hide resolved

src/wasm/wasm.cpp Outdated Show resolved Hide resolved

dhil added 2 commits November 13, 2024 15:14

Compute resume table sent types during expression construction.

c8d345a

Add switch lit test

0110dd5

Fix unreachable type printing

f418774

tlively reviewed Nov 18, 2024

View reviewed changes

src/wasm/wasm-binary.cpp Outdated Show resolved Hide resolved

src/wasm/wasm-ir-builder.cpp Outdated Show resolved Hide resolved

dhil added 3 commits November 20, 2024 16:28

Merge

3bedcb5

Read sentTypes off label types.

b1d65ce

dhil force-pushed the stack-switching branch from 82d683a to c549cd7 Compare November 28, 2024 11:23

Merge with upstream

6a20680

Fix lint

2a3fabe

Eliminate type immediates from stack switching instructions

d19763c

tlively reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stack switching proposal support #7041

Stack switching proposal support #7041

dhil commented Oct 30, 2024

tlively left a comment

tlively commented Nov 5, 2024

dhil commented Nov 5, 2024

dhil commented Nov 6, 2024

tlively left a comment

tlively Nov 6, 2024

dhil Nov 12, 2024

dhil commented Nov 18, 2024

tlively commented Nov 18, 2024

dhil commented Nov 28, 2024 •

edited

Loading

tlively commented Dec 2, 2024

dhil commented Dec 3, 2024

tlively left a comment

tlively Dec 3, 2024

tlively Dec 3, 2024

tlively Dec 3, 2024

Stack switching proposal support #7041

Are you sure you want to change the base?

Stack switching proposal support #7041

Conversation

dhil commented Oct 30, 2024

tlively left a comment

Choose a reason for hiding this comment

tlively commented Nov 5, 2024

dhil commented Nov 5, 2024

dhil commented Nov 6, 2024

tlively left a comment

Choose a reason for hiding this comment

tlively Nov 6, 2024

Choose a reason for hiding this comment

dhil Nov 12, 2024

Choose a reason for hiding this comment

dhil commented Nov 18, 2024

tlively commented Nov 18, 2024

dhil commented Nov 28, 2024 • edited Loading

tlively commented Dec 2, 2024

dhil commented Dec 3, 2024

tlively left a comment

Choose a reason for hiding this comment

tlively Dec 3, 2024

Choose a reason for hiding this comment

tlively Dec 3, 2024

Choose a reason for hiding this comment

tlively Dec 3, 2024

Choose a reason for hiding this comment

dhil commented Nov 28, 2024 •

edited

Loading