Skip to content

[BoS] Rename instructions #51

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

Merged
merged 2 commits into from
May 13, 2024
Merged

[BoS] Rename instructions #51

merged 2 commits into from
May 13, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented May 9, 2024

switch.call becomes stack.new_switch because it is the composition of
stack.new (not yet documented) and switch.

Similarly, switch.return becomes switch_retire because it performs a switch
and retires the previous active stack. We use "retire" instead of "return"
because "return" is not really an accurate description of what is happening.

`switch` becomes `stack.switch` because we are using the `stack` prefix for all
bag-o-stacks instructions.

`switch.call` becomes `stack.new_switch` because it is the composition of
`stack.new` (not yet documented) and `stack.switch`.

Similarly, `switch.return` becomes `stack.switch_retire` because it performs a
switch and retires the previous active stack. We use "retire" instead of
"return" because "return" is not really an accurate description of what is
happening.
@tlively tlively requested a review from fgmccabe May 9, 2024 05:00
Copy link
Member Author

tlively commented May 9, 2024

@@ -86,15 +86,15 @@ The return stack type must be of the form:

where $c is the index of a stack type.

>This affects which instructions are legal to perform; the switch.return instruction passes a null stack as the return stack, whereas the regular switch instruction never passes a null stack.
>This affects which instructions are legal to perform; the stack.switch_retire instruction passes a null stack as the return stack, whereas the regular strack.switch instruction never passes a null stack.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "strack.switch"

@slindley
Copy link
Collaborator

slindley commented May 9, 2024

These seem like sensible renamings to me. However, I know that @rossberg consciously tries to follow a particular naming convention that I've never quite internalised. For typed continuations we have cont.new, resume and suspend. I can never remember why we don't have the cont prefix for resume and suspend, but by analogy I'm not sure whether switch and switch_retire should have prefixes.

We should be wary of using the stack prefix as it may suggest that this is the canonical proposal for stack switching, which it is not (typed continuations are also in the picture - and much more developed). Perhaps the prefix should just be bos for now 😄

@rossberg
Copy link
Member

rossberg commented May 9, 2024

My reasoning was that Wasm so far is not using prefixes for instructions that actually affect control and instead treats those as more primary constructs, otherwise we'd already have e.g. func.call etc.

@slindley
Copy link
Collaborator

slindley commented May 9, 2024

And the distinction is that cont.new doesn't actually affect control directly, but rather creates a new stack in preparation for doing so?

@rossberg
Copy link
Member

rossberg commented May 9, 2024

Yes, it just allocates. This is my rationalisation of what Wasm has been using so far anyway.

@slindley
Copy link
Collaborator

slindley commented May 9, 2024

So IIUC based on that convention the instructions @tlively mentioned could be named bos.new, new_switch, switch, and switch_retire. The first just allocates whereas the latter three all affect control flow.

@tlively
Copy link
Member Author

tlively commented May 9, 2024

The only thing you can do with a stack that doesn’t involve a control flow transfer is allocate it, so stack.new will always be the only instruction with the stack. prefix under that scheme. Maybe that’s ok. I kind of like having the prefix on all the instructions despite the existing convention, but don’t feel strongly about it.

We should be wary of using the stack prefix as it may suggest that this is the canonical proposal.. . Perhaps the prefix should just be bos for now 😄

I agree we should be clear on this point, but I don’t think the instruction names are the right documentation mechanism. stack.new is much more intelligible than bos.new.

@slindley
Copy link
Collaborator

slindley commented May 9, 2024

The only thing you can do with a stack that doesn’t involve a control flow transfer is allocate it, so stack.new will always be the only instruction with the stack. prefix under that scheme. Maybe that’s ok. I kind of like having the prefix on all the instructions despite the existing convention, but don’t feel strongly about it.

I don't have a strong view either, but I think it would be helpful to use the same convention between proposals - so I'd suggest just using the prefix in the case of new as per @rossberg's convention.

We should be wary of using the stack prefix as it may suggest that this is the canonical proposal.. . Perhaps the prefix should just be bos for now 😄

I agree we should be clear on this point, but I don’t think the instruction names are the right documentation mechanism. stack.new is much more intelligible than bos.new.

Agreed. I don't think it matters too much as long as we don't run into name clashes (speaking of which let's make sure we allocate different byte codes for the instructions for the purposes of experimentation!).

However, it is worth remembering that the cont type in the typed continuations proposal and the stack type in the bag-o-stacks proposal both represent continuations, and in both cases these continuations are implemented as stacks. The key difference is that in the first case continuations / stacks are composable (i.e. can be spliced together), whereas in the second proposal they are not.

@dead-claudia
Copy link

Quick question: if stack.switch_call is equivalent to stack.new stack.switch as the initial comment implies, couldn't stack.switch_call be dropped to save opcode space? Implementors could fuse the two if it provides a perf advantage, so I don't see the value in keeping it around.

@fgmccabe
Copy link
Collaborator

fgmccabe commented May 9, 2024

In fact there are very good reasons to have the three instructions.

Originally there was only switch.call (stack.new.switch).

Ergonomically, having stack.new is nice, but not necessary.
stack.new is very difficult to implement efficiently (too much argument register shuffling)
and stack.new.switch is very important to some compiler writers.

So, if we were going to drop something, it would be stack.new.

Note that this is super early in the process.

@tlively
Copy link
Member Author

tlively commented May 9, 2024

@dead-claudia, another point in favor of an explicit stack.switch_call is that we generally try to avoid requiring engines to fuse operations to get reasonable performance. In Web engines, optimizing tiers will certainly fuse the operations anyway, but simpler engines or baseline tiers may benefit from having the explicit fused instruction.

@tlively
Copy link
Member Author

tlively commented May 9, 2024

How would we feel about keeping stack.new_switch instead of renaming it to new_switch? My rationale is that although this instruction does transfer control, that's not all it does. As much as we have a convention that control flow transfers are unprefixed, we also have a convention that allocation instructions are prefixed with what they allocate. I think the stack.new_switch name most clearly indicates that it is a composition of stack.new and switch.

Copy link
Collaborator

@fgmccabe fgmccabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@rossberg
Copy link
Member

rossberg commented May 10, 2024

@tlively:

My rationale is that although this instruction does transfer control, that's not all it does.

That doesn't really make a lot of sense to me, but at the same time, naming isn't something to get too hung up about at this stage.

@rossberg
Copy link
Member

rossberg commented May 10, 2024

@fgmccabe:

stack.new is very difficult to implement efficiently (too much argument register shuffling)

I suspect that depends on how regular semantics and arguments are. Such complexity can be an indication that there is a more fitting factorisation of primitives hidden underneath. As a data point, the corresponding primitive in the continuations proposal is quite straightforward to implement (and I believe that difference is largely orthogonal to other differences).

@fgmccabe
Copy link
Collaborator

@rossberg The same issue (argument register shuffling) would apply to cont.new.
To be clear, it CAN be implemented; but it will require generating a stack shuffle. Turbofan does something like this for tail calls; but in the case of stack.new/cont.new it is more difficult: in addition to register shuffling, we also have to spill the 'free' variables bound in stack.new/cont.new until the first resumption.
None of this happens if you just 'call on a new stack'.

@tlively
Copy link
Member Author

tlively commented May 13, 2024

I've updated this to rename stack.switch back to switch and stack.switch_retire back to switch_retire. I've kept stack.new_switch as-is. I'm going to go ahead and merge this now, but of course we can bikeshed naming more later.

@rossberg
Copy link
Member

@rossberg The same issue (argument register shuffling) would apply to cont.new. To be clear, it CAN be implemented; but it will require generating a stack shuffle. Turbofan does something like this for tail calls; but in the case of stack.new/cont.new it is more difficult: in addition to register shuffling, we also have to spill the 'free' variables bound in stack.new/cont.new until the first resumption. None of this happens if you just 'call on a new stack'.

That may be a V8 artefact. @dhil, @frank-emrich, isn't the cont.new implementation in Wasmtime rather straightforward?

@dhil
Copy link
Member

dhil commented May 17, 2024

That may be a V8 artefact. @dhil, @frank-emrich, isn't the cont.new implementation in Wasmtime rather straightforward?

Yes, cont.new is straightforward. No problems. Though, we have to do the "register shuffle" in resume if and only if cont.bind has been called on the continuation with a nonzero number of arguments previously.

dhil pushed a commit to dhil/wasm-stack-switching that referenced this pull request Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants