Skip to content
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

Branch-like structured control flow #98

Closed
wants to merge 2 commits into from
Closed

Conversation

sunfishcode
Copy link
Member

This PR is initial work pursuant to the current discussion around WebAssembly/design#299, adding the new low-level operators suggested by the discussion while preserving the existing high-level operators.

I took @rossberg-chromium's suggestion to use syntax sugar to keep the core code simple, though I instead implemented if/else/forever/break (forever was previously loop in the spec repo, but it's forever in the design repo) as syntax sugar in terms of br/br_if/br_unless because it makes the core spec simpler.

The one exception is switch, which is not yet implemented as syntax sugar, but only because I haven't gotten around to it, and wanted feedback first.

I also took the liberty of adding a bit of extra sweetener to the syntax sugar for the high-level opcodes. One can now use break on a forever loop or switch without wrapping it in an explicit label/block, which more closely resembles the languages this syntax sugar is inspired by.

@kg
Copy link
Contributor

kg commented Oct 1, 2015

Not needing to explicitly label all loops/switches is a big improvement, so I'm in favor of that. Not super thrilled by br/br_if/br_unless still, but your move to implement the existing ops as sugar is a good way to mitigate some of the concerns there, so smart stuff.

I'll do a detailed review of this PR later...

@ghost
Copy link

ghost commented Oct 1, 2015

Does 'syntax sugar' mean operations that are only implement in the syntax, and that there would not be binary opcode for them? This would seem fine to me as the consumer needs to recognize either pattern anyway and having a canonical representation might even make it easier.

@rossberg
Copy link
Member

rossberg commented Oct 1, 2015

I took @rossberg-chromium https://github.com/rossberg-chromium's
suggestion
WebAssembly/design#299 (comment)
to use syntax sugar to keep the core code simple, though I instead
implemented if/else/forever/break (forever was previously loop in the
spec repo, but it's forever in the design repo) as syntax sugar in terms
of br/br_if/br_unless because it makes the core spec simpler.

I take issues with that choice. You are trading 3 core primitives for 5,
while also making the checking and evaluation rules more complicated. How
is that simpler? I think this patch proves pretty much the opposite.

@rossberg
Copy link
Member

rossberg commented Oct 1, 2015 via email

@sunfishcode
Copy link
Member Author

br_unless is essentially the same as br_if; I only added to make it trivial to do low-level transformations that reverse the direction of branches. I could factor out br_if and br_unless into a common AST node, or even remove br_unless altogether, if the total number of AST nodes is the issue.

The most interesting example though is br_switch which has a much simpler implementation than switch.

@lukewagner
Copy link
Member

I just wanted to point out my comment in #99 which I think would also apply to this PR. Basically, after Andreas adds the new "sugar" AST to spec/, I think that would be the place to add all these new ops and then we can hash out which ops should go into the kernel AST.

| "br_if" { BRIF }
| "br_unless" { BRUNLESS }
| "if" { IF }
| "else" { ELSE }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ELSE token used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Good spot.

@AndrewScheidecker
Copy link
Contributor

Why keep both forever and loop?

Why keep both break and br?

If we have both a switch and a brswitch, I think I'd prefer brswitch to be called something more like br_table or br_match. However, I don't think there's much point in keeping the existing switch: without default falling through to other cases, it can't encode a C-style switch, so I expect most producers would fall back to generating a brswitch.

@sunfishcode
Copy link
Member Author

Why keep both forever and loop?

forever includes an implicit branch back to the top. In this PR, loop does not, you have to do an explicit branch if you want a branch. We don't actually need both. But we have so far been unable to agree, which has lead to the present compromise of having both.

Why keep both break and br?

In this PR, break is part of the syntax sugar suite; it can implicitly find the nearest enclosing loop or switch, skipping other nested constructs, as break does in C-like languages. br on the other hand is just a branch that can go to any label.

If we have both a switch and a brswitch, I think I'd prefer brswitch to be called something more like br_table or br_match.

Recently there's been discussion of using dense arrays of case values rather than sparse, and requiring the array to start at zero, which I'm tentatively thinking make sense for br_switch. With this change I think renaming br_switch to br_table sounds good.

@titzer
Copy link
Contributor

titzer commented Oct 2, 2015

FWIW, in v8-native prototype break can target a loop, a block, or a switch, and a continue can only target a loop. Both break and continue reference a depth from which to break out of, and that depth counts all of those constructs. In that sense, the label declaration is implicit.

I implemented a couple variants of switch, one with default fallthrough and one without. It's only a couple lines of code difference between the two. A general switch with labels and default (especially in the middle) is more work, and without some examples of typical switch constructs in programs, it's tough to tell what the right tradeoff is yet, since I think fallthrough in general switches will result in some ugly if-cascades.

@binji
Copy link
Member

binji commented Oct 7, 2015

I'm not a huge fan of this new syntax. Having labels defined after use is not great (though I suppose we already do that for functions). Using label indexes seem non-intuitive now too. Previously the label index corresponded to break depth, counting up from zero. How does it work now?

Also seems weird to have loop require an explicit branch. This means you can fall off the end of a loop or branch backward to an outer loop, right? It seems that the only difference between loop and block is the location of the label?

The syntax also seems to encourage a somewhat bizarre layout scheme where the blocks are flattened and the labels are dedented. This is very apparent in the block cascade of the br_switch statement.

@sunfishcode
Copy link
Member Author

Label indices in this proposal work in the same way as they do in the current ml-proto code, corresponding to break depth in exactly the same way. You can use a name in place of a label in the current ml-proto code too.

Block labels at the bottom is something that looks natural to me, because that's where the code goes when it branches to that label. I find JS labeled blocks awkward because I have to find the label at the top and them jump down to the bottom to see where the code is going. But if people prefer to declare the labels at the top, it's easy to implement.

Yes, the only difference between block and loop in this proposal is the location of the label. Requiring an explicit branch to stay in the loop is very natural from a compiler perspective. If it would help people understand what's going on here, I'd be ok renaming 'loop' to something else.

The big picture is that this proposal is inspired by compilers and low-level tools, where there is a desire for control transfers to be simple, unopinionated, and representable in a linear fashion (as this proposal is). It stops short of being a fully general CFG, because this lets us keep a hierarchical structure and the various nice properties that entails, but it's very easy to emit from a compiler with a CFG.

@sunfishcode
Copy link
Member Author

I'm not pursuing this PR presently, so closing.

@sunfishcode sunfishcode deleted the control-flow branch November 10, 2015 23:19
alexcrichton pushed a commit to alexcrichton/spec that referenced this pull request Nov 18, 2019
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this pull request May 11, 2020
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Mar 2, 2023
rossberg pushed a commit that referenced this pull request Nov 7, 2024
- Removed unnecessary/mismatching lookup of table/memory type in execution prose
- Added missing result type lookup in formal rule for table.size and memory.size
- Fixed computation of -1 result value for table.grow and table.size to work for i64
- Some fixes around specification of text format for inline elements/data shorthand
- Fixed matching rules for tabletype/memtype to enforce same address type

Split out from #1839
dhil added a commit to dhil/webassembly-spec that referenced this pull request Dec 3, 2024
* Fix validation of `switch`.

The validation of `switch` was not working as intended when the
"switcher" and "switchee" had different continuation type immediates.

The fix is to replace the current continuation from the argument list
with the switched-to continuation. The previous thing happened to be
working when the "switcher" and "switchee" used the same continuation
type immediate.

I also noticed a bug in the testsuite runner. It was not running the stack switching tests since commit WebAssembly/stack-switching@70086b9. I have added the "stack-switching" subdirectory to the test script runner such that the tests are now being run again by `make test`.

Resolves WebAssembly#98.

* Update interpreter/valid/valid.ml

Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>

---------

Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
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.

7 participants