1242342 - wasm: If/Else control flow opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

wasm has much fewer controp flow opcodes than the current internal IR has:

- br (branch to an enclosing label)
- brIf
- loop
- tableswitch

The asm.js frontend needs to emit these opcodes instead of what it emits now.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Change semantics of IfElse (wip)

This updates If/IfElse semantics to be able to yield a value. We can now remove Expr::Ternary and the code generating it.

Not asking for review yet, there might be some bugs wrt yielding that will be shown only when it's implemented in wasm.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Replace Ternaries with IfElse that yield

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Wrap asm.js statements inside a single block

We can't know what's the expected type of control flow is without looking at the return type of the current function. Obviously, this is currently wrong for most asm.js functions, as most asm.js (expression) statements don't have the same type as the function's return type. So we need to wrap all asm.js statements inside a single block. This patch does that.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3. Add If / IfElse to WebAssembly

With a bunch of tests to make sure that yielding a value works from if_else but not from if.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8714935 [details] [diff] [review]
1. Replace Ternaries with IfElse that yield

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

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2472,5 @@
>      // for the entire if/else-if chain).
>      BlockVector thenBlocks;
>  
> +    bool yield = expected != ExprType::Limit;
> +    MOZ_ASSERT_IF(!hasElse, !yield);

'yield' is a bit confusing (esp given exception-handling meaning).  What about just testing 'expected != ExprType::Limit' everywhere which conveys "the result value is used".

I just noticed and confirmed in a test patch: we don't ever need to pass ExprType::Limit, we can pass ExprType::Void in CheckExprStmt and switch all the ExprType::Limit checks to ExprType::Void.  This all mirrors the wasm type system which uses and expected type of 'void' to mean "don't care what your result value is".

@@ +2504,5 @@
> +    f.switchToElse(elseOrJoinBlock);
> +
> +    Expr nextStmt = f.peekOpcode();
> +    if (nextStmt == Expr::If || nextStmt == Expr::IfElse) {
> +        hasElse = nextStmt == Expr::IfElse;

Probably good to re-MOZ_ASSERT_IF(!hasElse, expected == ExprType::Void).

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8714936 [details] [diff] [review]
2. Wrap asm.js statements inside a single block

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

Given the extension we currently have where wasm::IonCompileFunction can emit N exprs instead of just 1 body expression of wasm AND the fact that all asm.js functions either (1) have explicit typed returns, (2) return void, wouldn't it be sufficient to change EmitExprStmt to pass ExprType::Void as the expected type (as coincidentally suggested in the previous patch's review)?  This will allow any expression and return statements are checked against sig.ret().

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8714937 [details] [diff] [review]
3. Add If / IfElse to WebAssembly

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

::: js/src/asmjs/Wasm.cpp
@@ +285,5 @@
> +        return DecodeExpr(f, ExprType::Void);
> +    }
> +
> +    return DecodeExpr(f, expected) &&
> +           DecodeExpr(f, expected);

I think you could rephrase this as:

  return DecodeExpr(f, ExprType::I32) &&
         DecodeExpr(f, expected) &&
         (hasElse
          ? DecodeExpr(f, expected)
          : CheckType(f, ExprType::Void, expected);

This more directly expresses the type checking in the spec wherein Ast.If is first desugared to an if-else where the else is a Nop and type checking a Nop does a CheckType(None, expectedType).

::: js/src/asmjs/WasmText.cpp
@@ +1274,5 @@
> +        return e.writeExpr(Expr::If) &&
> +               EncodeExpr(e, ie.cond()) &&
> +               EncodeExpr(e, ie.ifBody());
> +    }
> +    return e.writeExpr(Expr::IfElse) &&

Copying the example of EncodeCall, perhaps you could store an Expr in WasmAstIfElse which would then allow this body to be written:

  return e.writeExpr(ie.expr()) &&
         EncodeExpr(e, ie.cond()) &&
         EncodeExpr(e, ie.ifBody()) &&
         (!ie.hasElse() || EncodeExpr(e, ie.elseBody()));

Additionally, ParseIfElse could take this Expr as a parameter so it didn't need the 'hasElse' bool.

::: js/src/jit-test/lib/wasm.js
@@ +1,2 @@
> +if (!wasmIsSupported())
> +    quit();

Ah hah, good one.

::: js/src/jit-test/tests/wasm/basic.js
@@ +209,5 @@
>  assertEq(wasmEvalText(code.replace('BODY', '(call 0)'), imports)(), 3);
>  assertEq(wasmEvalText(code.replace('BODY', '(call 1)'), imports)(), 4);
> +
> +// ----------------------------------------------------------------------------
> +// if_else

Perhaps we could move this out and start a new basic-control.js to mostly just have the module-level constructs in basic.js?

@@ +218,5 @@
> +assertErrorMessage(() => wasmEvalText('(module (func (local f64) (if_else (get_local 0) (i32.const 1) (i32.const 0))))'), TypeError, mismatchError("f64", "i32"));
> +wasmEvalText('(module (func (local i32) (if (get_local 0) (nop))) (export "" 0))');
> +wasmEvalText('(module (func (local i32) (if_else (get_local 0) (nop) (nop))) (export "" 0))');
> +
> +// Yielded types are consistent

I'd avoid using "yield".  Perhaps just use "expression types" or "expression values".

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8714936 [details] [diff] [review]
2. Wrap asm.js statements inside a single block

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

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2937,5 @@
>  
>  static bool
>  EmitExprStmt(FunctionCompiler& f, MDefinition** def, LabelVector* maybeLabels)
>  {
> +    return EmitExpr(f, ExprType::Void, def, maybeLabels);

Answering your question about this patch: this in fact replaces ExprType::Limit with ExprType::Void, but this isn't the most important change...

@@ +2965,5 @@
>              return false;
>  
>          MDefinition* last = nullptr;
>          while (!f.done()) {
> +            if (!EmitExpr(f, f.sig().ret(), &last))

The most important thing in this patch is this line: if we don't make this change, then (func (result i32) (if_else 1 1 0)) will return garbage. Indeed, EmitExprStmt will call into EmitIfElse with Void (meaning "the value is not used"), then *last == nullptr, and we don't return the int32 while we should. So we need to call EmitExpr with the right expected return type, *at least for the last expression*. But as you say, WasmIonCompile can emit N expressions, and we don't know how many are remaining at this point. Ideally, we'd just do something as EmitBlock (expect Void for all sub-expressions and the expected type for the last sub-expression), but we are under the limitation of not knowing how many expressions remain.

In wasm, it's nice as we only have a single parent subexpression, so we can assume it is the last one.
That's an issue for asm.js, where expression statements can have another type than the return type:

function f() {
  var x = 0;
  x = x + 1 | 0; // i32 set_local
  return +13.37; // returns double
}

Hence the need to wrap all asm.js statements within a single block.

(Thinking about it, the condition below "IsVoid(f.sig().ret()) || !last" is probably wrong; the last part should be put away, and instead we should MOZ_ASSERT(last) in the last branch. Also, with this change, there should be only a single expression so we can probably remove the entire loop. Will update my patch.)

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Generate function sub-expressions wrt compilation mode

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8715335 [details] [diff] [review]
2. Generate function sub-expressions wrt compilation mode

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

Beautiful, thanks!

::: js/src/asmjs/WasmBinary.h
@@ +723,5 @@
>  // lives only until it is fully compiled.
>  class FuncBytecode
>  {
>      // Function metadata
> +    bool isAsmJS_;

Can you hoist ModuleKind from WasmModule.h into WasmTypes.h and use that instead as the memvar and ctor arg type?

::: js/src/asmjs/WasmGenerator.cpp
@@ +488,5 @@
>                                       Move(fg->locals_),
>                                       fg->lineOrBytecode_,
>                                       Move(fg->callSiteLineNums_),
> +                                     generateTime,
> +                                     isAsmJS());

With the other change, this would turn into module_->kind.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9aec0bc1163

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfbbaeadb0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4044ca82798
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ea602c3c93

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3cfbbaeadb0b
https://hg.mozilla.org/mozilla-central/rev/c4044ca82798
https://hg.mozilla.org/mozilla-central/rev/e8ea602c3c93