1456006 - Add SwitchEmitter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

Just like IfThenElseEmitter (bug 1147371) and TryEmitter (bug 1322069),
having SwitchEmitter for switch-case-default should help bug 1455548.

Comment 1

[Tooru Fujisawa [:arai]]

Considering BinAST streaming compilation,
the current switch statement's bytecode sequence is really troublesome,
because we emit all case expressions first, and then all case/default bodies later.
this at least doesn't match the order of the nodes appears in the source code,
and I'm not sure modifying BinAST spec just for this purpose is a good thing.

also we're using 2 different opcodes, JSOP_CONDSWITCH and JSOP_TABLESWITCH,
and JSOP_TABLESWITCH is an optimization for the case that all case expressions are integer in small range.
so we at least have to parse all case expressions in order to figure out if JSOP_TABLESWITCH is usable,
and we might have to go back to the first case expression if not, or remember all expressions in memory.

SwitchEmitter for current bytecode emitter is ready tho, it requires on-memory AST or something similar to figure out
what method to call, depending on case expressions.

Comment 2

[Tooru Fujisawa [:arai]]

filed https://github.com/binast/ecmascript-binary-ast/issues/35

Comment 3

[Tooru Fujisawa [:arai]]

to be clear, here's order for both of them:

bytecode order:

  expr

  CONDSWITCH

  case_expr1
  case_expr2
  ...
  case_exprN

  case_body1
  case_body2
  ...
  case_bodyN

or

  expr

  TABLESWITCH with integer ranges calculates from case_expr1...N

  case_body1
  case_body2
  ...
  case_bodyN

and JS source order:

  switch (expr) {
    case case_expr1:
      case_body1
    case case_expr2:
      case_body2
    ...
    case case_exprN:
      case_bodyN
  }

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add reportError and reportExtraWarning variants to receive offset instead of ParseNode.

While reporting error from helper classes which doesn't use ParseNode, we need to pass possible offset directly, instead of ParseNode which has pn_pos.
I've added variants of reportError and reportExtraWarning which takes Maybe<uint32_t> instead of ParseNode*.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add SwitchEmitter.

SwitchEmitter handles bytecode generation of swtich statement, both table switch and cond switch,
also takes care of scope for lexical binding inside case bodies,
and TDZCheckCache for case expressions and case bodies.

SwitchEmitter::TableGenerator handles the calculation of table for table switch, including falling back to cond switch in case the range doesn't match the requirement.

The usage is in the comment in SwitchEmitter.h.

To support emitting bytecode without ParseNode, the scratch space for offset is moved from ParseNode.pn_u.binary.offset to SwitchEmitter's field.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8992509 [details] [diff] [review]
Part 1: Add SwitchEmitter.

Review of attachment 8992509 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/SwitchEmitter.h
@@ +290,5 @@
> +        // Returns the `caseIndex` value for SwitchEmitter::emitCase and
> +        // SwitchEmitter::emitCaseBody parameters.
> +        // This method can be called only if `isValid()` is true.
> +        // Otherwise the index of the case inside the switch (excluding the
> +        // default case) should be passed to those methods.

Please ignore these comments, they're for previous API design.

the correct one should be:

  // Returns the index in SwitchEmitter.caseOffsets_ for
  // table switch.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8992509 [details] [diff] [review]
Part 1: Add SwitchEmitter.

Review of attachment 8992509 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2405,5 @@
>                  continue;
>              }
> +
> +            if (!tableGen.addNumber(i))
> +                return false;

Bleh, so once we determine we can't tablegen, this whole loop very elaborately just searches for a default, and that's all it does?  We should figure out if there's some way to store the had-a-default bit somewhere, maybe, sometime.

@@ +2418,3 @@
>              return false;
>      } else {
> +        if (!se.emitCond(caseCount))

The case-count validation could be moved into its own function, called before the |bool isTableSwitch| declaration, couldn't it?  Seems nicer to do that in one place than two, and before several other initializations that end up getting thrown away if there are too many cases.  SE::validateCaseCount or something.

::: js/src/frontend/SwitchEmitter.cpp
@@ +36,5 @@
> +
> +    if (caseValue < low_)
> +        low_ = caseValue;
> +    if (caseValue > high_)
> +        high_ = caseValue;

Could write these as

  low_ = std::min(low_, caseValue);
  high_ = std::max(high_, caseValue);

maybe.  Feels a little more compact, clearer about making further-minimal/maximal as needed.  IMO.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74b3d9d91b3e2432fd30a1c36a140ad730076ba6
Bug 1456006 - Part 0: Add reportError and reportExtraWarning variants to receive offset instead of ParseNode. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2cfbb2fcc82ea270d177a3200c6d69b97bc0053
Bug 1456006 - Part 1: Add SwitchEmitter. r=jwalden

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/74b3d9d91b3e
https://hg.mozilla.org/mozilla-central/rev/d2cfbb2fcc82