1249531 - Baldr: add text format and encoding support for control operators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: other-control-flow

Bug 1246116 will make the actual encoding/semantic changes for Br/BrIf/Loop/Tableswitch; this bug just adds br/br_if/loop/tableswitch to the s-expr text language.  With this patch, we can parse full Binaryen output (at least bananabread).

Comment 1

[Michael Bebenita [:mbx]]

Comment on attachment 8721157 [details] [diff] [review]
other-control-flow

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

Looks great! Minor nits and table switch bug need to be addressed. Excited to see that we can parse large Binaryen files now.

::: js/src/asmjs/WasmText.cpp
@@ +2602,5 @@
>  
> +static WasmAstTableSwitch*
> +ParseTableSwitch(WasmParseContext& c)
> +{
> +    WasmAstExpr* index = ParseExpr(c);

TableSwitches also have breakNames.

@@ +2614,5 @@
> +
> +    WasmRefVector table(c.lifo);
> +
> +    while (c.ts.getIf(WasmToken::OpenParen)) {
> +        if (!c.ts.match(WasmToken::Br, c.error))

What about WasmToken::Case, follow-up bug?

@@ +3184,5 @@
> +
> +static bool
> +ResolveBranch(Resolver& r, WasmAstBranch& br)
> +{
> +    if (!br.target().name().empty() && !r.resolveTarget(br.target()))

Since the |if(!x.name().empty() && !r.resolveXXXX()) pattern is so common, it would be nice to move the name().empty() check into the resolveXXX() function. Here, and everywhere else.

@@ +3321,5 @@
>  }
>  
>  static bool
> +ResolveTableSwitch(Resolver& r, WasmAstTableSwitch& ts)
> +{

Need to push the TableSwitch breakName here.

Comment 2

[Luke Wagner [:luke]]

(In reply to Michael Bebenita [:mbx] from comment #1)
> TableSwitches also have breakNames.

Those are only really useful when you have cases; along with the proposal to remove cases we're proposing to remove the break label too.

> What about WasmToken::Case, follow-up bug?

We'll be proposing to remove them next week :)

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e20bdd12dd1

Comment 4

[Luke Wagner [:luke]]

Attachment: measure

Not for landing, but here's a patch I wrote that adds some basic instrumentation to WasmText.cpp to print AST node percentage as well as the number of bytes spent on various nodes' immediates.

Comment 5

[Michael Bebenita [:mbx]]

Neat. Also take a look at: https://github.com/WebAssembly/binaryen/pull/204

You can do something like |./binaryen-shell/ x.wast --metrics| to dump stats, for example:

https://pastebin.mozilla.org/8860409

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e20bdd12dd1

Comment 7

[Luke Wagner [:luke]]

Attachment: measure