-
Notifications
You must be signed in to change notification settings - Fork 694
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
Flexible control-flow operators. #427
Conversation
This introduces `br`, `br_if`, and so on, while also preserving the high-level `if` and `if_else` operators. It also converts `switch` into `tableswitch` and generalizes it to support labels in enclosing scopes.
5fde46e
to
2e1ccb1
Compare
Updated PR to be independent of statements=expressions. |
I'd like to understand the motivation for tableswitch better. Isn't that simply a more convoluted way of writing a C-style switch? As far as I can tell, it is completely equivalent, the only differences being stylistic. Namely, (1) all cases have to be written with an indirection through an extra label instead of using (1) and (2) are effectively just changes in syntax, not semantics. Of course, this representation makes sense on the encoding level, but on the AST level, it just makes the construct (significantly) more verbose and more complicated to describe and spec. (3) on the other hand is just syntactic sugar, analogous to So in terms of AST semantics and spec factorisation, tableswitch is strictly inferior to a C-style switch, while being functionally equivalent. I think we should treat the table representation as an encoding concern, not a semantics concern. |
This It also has a notable restriction in that it requires the index space to be zero-based and dense. This makes it more directly correspond to an actual jump table, as reflected by the rename to |
Yes, I understand, but as I tried to point out above, that seems more like an encoding consideration, and not like something that is necessarily relevant on the semantic abstraction level of the AST. |
I don't see how the semantics of where a |
As mentioned above, the ability to jump to an external label is merely syntactic sugar over a break (and limited one, because there is no way you can make it transfer a value). As such, it is primarily an encoding consideration as well. Can you explain what you are referring to as index space, and how it affects the operand? |
We discussed
|
@rossberg-chromium One can express any reasonable switch-like construct as syntax sugar on top of any other plus extra nodes. As such, I don't see why this is a reason to prefer any one over any other. Also, Statements == Expressions is an orthogonal concern. This The index space of |
@qwertie, yes, I was comparing with a C-style switch, which I'm happy moving towards. Density is an independent consideration. @sunfishcode, re index space, that's what I guessed, but then I still don't understand your earlier remark on that. It may help to point out that we are actually discussing syntax here rather than semantics. I am fine with the goal of increasing the flexibility of the switch operator. But I am puzzled how the explicit syntactic indirection through extra labels helps at the AST level. Let's be concrete. Consider a switch as described in this proposal:
Here is how I would design the concrete S-expr syntax:
That is expressing exactly the same structure but with much less repetition. You can still represent it as a table of labels in the binary encoding, but as a textual or AST-level representation it is superior in just about every way I can imagine, for both producers and "consumers", including the spec. Note that the density requirement is independent from this choice of syntax. I don't see a particularly strong reason to require listing the Note also that I even turned the break to the outer label into special syntax for So, again, this is really about syntax, not semantics. AFAICT, the latter way of structuring the operator syntax is more convenient on all levels. I'd like to understand what advantage you see in the former notation. Or would you be fine with the latter as well? |
I've now added a patch to this PR converting it to what I think you're asking for. Is this what you had in mind? In my opinion, this patch makes the prose harder to read for humans, makes it harder for me to convince myself that the semantics aren't incomplete or inconsistent, and it puts greater distance between the design-document AST representation and what I might imagine we'll want in the binary encoding. |
@sunfishcode, cool, thanks! And for your pleasure, I already implemented it: WebAssembly/spec#153 :) |
6b70f43
to
2e1ccb1
Compare
I decided I didn't like it, so I removed it now. |
I see. :( Well, please have a look at the spec PR. AFAICS, with this structure, it is as simple as it will get. Tests also remain more readable. I hear what you are saying re prose and distance to binary encoding, but I think those are far outweighed by familiarity and structural simplicity, respectively. |
References to labels must occur within an *enclosing construct* that defined | ||
the label. This means that references to an AST node's label can only happen | ||
within descendents of the node in the tree. For example, references to a | ||
`block`'s label can only happen from within the `block`'s body. In practice, |
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.
"happen from within" => "occur within"
This mostly looks good modulo some wordsmithing. |
@rossberg-chromium You've mentioned a C-style switch as a basis for the tableswitch design. Here we are being a little bit more explicit with labels, which pushes a bit of work up to producers, at the gain of being explicit for backends and engines. Is this a reasonable tradeoff in your opinion? Or are you proposing that the syntax of the AST is C-like and the binary encoding is table-switch like? |
@sunfishcode Have you done much research into how this would impact pre-compression and post-compression file size (in the asmjs packer, for example?) The significant changes to ASTs this implies could mean a big file size regression (or a big file size win, in which case that alone is a compelling reason to make your changes!) |
@kg Looking at some asm.js code of some miscellaneous applications, |
74029cc
to
74c21c8
Compare
Wordsmithing changes applied. |
The question is about the binary encoding. Are you saying |
@kg I think this has been answer elsewhere - syntax sugar is a spec matter and there are still opcodes for the sugar. |
On 26 October 2015 at 18:55, titzer [email protected] wrote:
Yes, the latter. There is no real benefit in introducing explicit labels as |
WebAssembly/spec#153 now matches everything described by this PR (I think), modulo the notation for tableswitch. I did some more pondering over the latter, esp looking into how the proposed notation would affect the spec. Honestly, it would be quite ugly. :( In addition to tracking the extra indirection, it would require distinguishing two different kinds of (raw) labels in the AST (since tableswitch addresses its inner targets differently from external targets) as well as different scoping rules for those labels' symbolic representation. In other words, my assessment still stands that this notational factorisation for tableswitch is suboptimal. Unless we have strong technical reasons to do it that way, I highly recommend more streamlined notation. @sunfishcode, do you think you can still warm up to that? |
My attempt above at implementing what I thought you were asking for made the prose about twice as long, and I've now discovered, as I feared, that it did have a bug hidden in the extra complexity: it didn't address whether case and default could fall through to case_br and default_br. Right now, I am very confused as to the intended relationship between AstSemantics.md, ml-proto's s-expression syntax, what we might expect to call the binary format AST, and what we might expect in the actual binary format. This confusion is making it difficult for me to figure out what you're asking for here, and what it means for other parts of the system. I propose we merge this PR as a starting point, and then you can submit a new PR after it proposing the specific changes what you would like to see. I think that will help me understand better how you want this to look, rather than having me try to write up what I think you want. Does that make sense? |
@sunfishcode, okay, sounds good to me. |
Ok. Merging. |
Flexible control-flow operators.
This introduces
br
,br_if
, and so on, while also preserving thehigh-level
if
andif_else
operators. It also convertsswitch
intotableswitch
and generalizes it to support labels in enclosing scopes.