Implement changes to control flow operators

[rossberg]

Okay, here is a new implementation of the control operators (as an alternative to #153) that approximates the current description in the design doc, including the more verbose tableswitch. I managed to keep most of the complexity out of the kernel. The price to pay is that the desugaring no longer just consists of local expansions, it actually has to rewrite the case list to handle label targets. That is not ideal but probably bearable. The rest of the complexity is in the parser, where case labels get their own namespace with somewhat subtle scoping.

The abstract syntax of the jump table is more heavyweight than you envisioned, but don't say I didn't tell you so. ;)

[rossberg]

Btw, here is an idea for slightly simplifying the notion of default: instead of making it a target separate from the jump table, why don't we simply say that the index operand is clamped to the size of the table? That would be equivalent, since you could then just make the default the last element of the table, but it removes one minor concept. WDYT?

[rossberg]

See control...control.clamp for the latter suggestion.

[titzer]

On Thu, Nov 5, 2015 at 10:35 AM, rossberg-chromium <notifications@github.com

wrote:

Btw, here is an idea for slightly simplifying the notion of default:
instead of making it a target separate from the jump table, why don't we
simply say that the index operand is clamped to the size of the table? That
would be equivalent, since you could then just make the default the last
element of the table, but it removes one minor concept. WDYT?

Making it the last target of the table sounds fine to me, but I'd avoid
wording about "clamping" and just say directly that the last entry is the
default target.

—
Reply to this email directly or view it on GitHub
#163 (comment).

[sunfishcode]

Part of me was hoping we could use a LISP-style array literal, #( <target>* ) or [ <target>* ] depending on the dialect, but I'm ok if we can't because of generic S-expression parsers.

[sunfishcode]

This looks good to me at an initial readthrough, with some minor comments above. We can iterate from here.

I'm not partial to putting the default in the table. In my mental model, tableswitch desugars into first testing whether the index is out of bounds, and performing the jump table operation otherwise, so it feels more natural to keep the default separate. However, I agree that the semantics can work that way.

On that note, part of me would like to get rid of the default altogether and have tableswitch just trap if the index is out of bounds. It'd be great when one can prove no default is needed (eg. in a language with a strong type system). The main downside though is that if an application needs a default and does its own range check, and the tableswitch has its own builtin range check under the hood, we'd get two range checks. A clever JIT could optimize away the second by noticing it's covered by the first, or use virtual memory to make the second check "free", or other magic. However on today's hardware with a simple JIT, we'd get two range checks. I'm wondering if the part of me that wants this can convince the part that's concerned about simple JITs that it's worth doing though. If other people like it, perhaps we could explore the idea. What do you think?

[rossberg]

@sunfishcode, removed dead code, thanks for finding!

[rossberg]

Once this is landed, I'll create a separate PR for the "clamping" default, so we can have the discussion there.

As for a trapping switch, I'd like that even better. @titzer, do you think the scenario @sunfishcode mentions would be easy enough to optimise for something like v8-proto?

[titzer]

Not sure that trapping tableswitch is a good idea. As @sunfishcode
mentions, that means an application does its own clamping to the default or
end of the table. The tableswitch basically looks like "switch(a >= size ?
size -1 : a)", i.e. a branch on the phi of a branch. Then the VM adds its
check before the switch, which is the output of the applications' check.
The only way to know the second check is in range is to do range analysis
(or a silly pattern match). If you want trap-on-out-of-bounds (which I
think will be rarer), it's easier to just make the default case have an
unconditional trap. You get the basically the same code for
trap-on-out-of-bounds and avoid needing the extra application-level
clamping on default-on-out-of-bounds.

On Thu, Nov 5, 2015 at 11:25 PM, rossberg-chromium <notifications@github.com

wrote:

Once this is landed, I'll create a separate PR for the "clamping" default,
so we can have the discussion there.

As for a trapping switch, I'd like that even better. @titzer
https://github.com/titzer, do you think the scenario @sunfishcode
https://github.com/sunfishcode mentions would be easy enough to
optimise for something like v8-proto?

—
Reply to this email directly or view it on GitHub
#163 (comment).

[sunfishcode]

@titzer I don't think producers would use a clamp. LLVM, for example, will do something like this:

   if (a < size) {
      switch (a) {
      ...
      }
   } else {
      default_stuff_here();
   }

because this avoids routing the default path through the indirect branch, which is quite nice if the default path is hot, which it sometimes is in C/C++. (This is also related to why I'm not fond of thinking about the default being inside the table.)

If the JIT also lowers to this form instead of a clamp, we could end up with code effectively like this:

    if (a < size) {
       if (a < size) {
          switch (a) {
          ...
          }
       } else {
          trap();
       }
    } else {
       default_stuff_here();
    }

This way it's just a matter of recognizing that a < size is true on the true side of an a < size check.

[rossberg]

So anybody not okay with landing this PR?

[titzer]

lgtm

[sunfishcode]

lgtm

[dschuff]

Fixed up the conflicts with unreachable, merging based on lgtms and IRC with @sunfishcode

[rossberg]

Thanks!