1286517 - wasm: Implement Global section

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

A recent version of the spec added Globals to wasm.

If I understand correctly, wasm Globals differ from asm.js globals in that they can be exported (in addition to being imported) and then their state is shared among Instances that import/export these. To make this work, globals will need their own allocations (as function tables do now).

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

A very wip-y patch that prepares the machinery for adding globals to wasm (without testing them yet -- some things are known to not be working). This passes all the asm.js tests so far.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. renaming.patch

A renaming patch, first.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

Still a WIP:
1 - locally defined globals are in a section placed before global imports;
2 - imported and local globals share the same index space
3 - we can re-export any const global (be it imported or local).

Because of these 3 points, we need to keep track of the first non-local global (for checking we have the right number of imports, and for exports), and this makes things a bit complicated, so I may try other things. (plus, this needs tests and text-to-binary support)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa1eab6436ba
Rename AsmJS{Load,Store}GlobalVar to Wasm{Load,Store}GlobalVar; r=luke

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f17f746d10
Fix assertion in ARM's codegen; r=luke over irc;

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because ARM was still busted after the followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/86f32ce6a344

https://treeherder.mozilla.org/logviewer.html#?job_id=32245602&repo=mozilla-inbound

Comment 7

[Luke Wagner [:luke]]

Looking at the logs, the failure is in some mercurial operation.  FWIW, this try push I made (testing my own patches based on top of aa1eab6436ba) came up green on SM(arm):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=21033d62c6fb

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Yeah, the last command that timed out is:

    Trying to use bundle https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/bundles/mozilla-inbound.hg

But let's better be sure! Building and running all jit-tests in tbpl mode on my machine, for the ARM simulator, shows no failures. Let's see what try says...

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3faf6a4932c1
Rename AsmJS{Load,Store}GlobalVar to Wasm{Load,Store}GlobalVar; r=luke

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

Still a wip.

This patches:
- implements i64 global loads and stores on x64, with a proper interface for other platforms.
- adds text-to-binary (and binary-to-ast) conversions.
- implements imports/exports and passes *most* tests

Still a few things to do:
- export an int64 in testing mode
- pass all the tests as defined in globals.js

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3faf6a4932c1

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: folded.patch

Folded patch, to be cut in smaller parts.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.renaming-opcodes.patch

This renames the opcodes {Load/Store}Global to {Get/Set}Global.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.i64-globals-codegen.patch

This implements x64 codegen for i64 globals.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.global-section.patch

Sorry for the big patch: this implements the Global section, uses it in AsmJS and checks it in Wasm. Next patch implements the text format counterparts.

It also:
- refactors imports code in AsmJS into a GetImports function, for symmetry with wasm.
- implements initializer expressions for initializing a global (from a constant or another known immutable global).
- moves without touching Create/Read I64Object to WasmModule.h/cpp because they're used in WasmInstance (callExport), WasmJS (GetImports) and WasmModule (CreateExportObject) now.
- adds annotated tests in globals.js

There's an invariant in Wasm mode (but not in AsmJS mode) that imported globals are located upfront in the globals array (because imports are read before global definitions); this is intensively used when importing/exporting the globals.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.text-format.patch

This implements the text format counterparts to be able to actually test globals. I guess the tests should actually be added to this patch, or the two patches need merging after review.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6.elem-segments-initializers.patch

This allows element segments initializers to use any kind of initializer expressions, so they can make use of get_global.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7.asmjs-constants-simple-globals.patch

A simple refactoring to make AsmJS use real globals for what's currently called ConstantLiteral. We're this close to killing ConstantLiteral as well, but it's still explicitly used in IsLiteralOrConst (in AsmJS.cpp), which is used for initializing variables to a constant value, or as an array / SIMD lane index (in IsLiteralIntOrConst).

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.global-section.patch

(diff: put the tests in the text-format patch, for consistency with order of patches)

See previous comment for explanation.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.text-format.patch

(diff: tests are now included in this patch)

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 8773742 [details] [diff] [review]
2.renaming-opcodes.patch

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

::: js/src/asmjs/WasmBinary.h
@@ +272,5 @@
>      I64Eqz                               = 0xba,
>  
> +    // Global access.
> +    GetGlobal                            = 0xc0,
> +    SetGlobal                            = 0xc1,

Ah, right, the PR currently reuses the opcode for CallImport, which we haven't removed yet.  We can slot them into place when we remove CallImport.  Also we'll probably "defragment" the whole opcode space soon.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 8773751 [details] [diff] [review]
4.global-section.patch

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

This is looking really good, I think you've found all the right cut points.  There's just one non-nit issue which is that initializer expressions must be for immutable global *imports*, not constants.  Thus, they derive their value at instantiation-time, like asm.js `function f(stdlib, ffis, buffer) { "use asm"; var i = ffis.x|0; }`.  This is actually the root motivation for adding globals to begin with: it allows us to give elem/data sections' offset field an instantiation-time value (which is necessary for dynamic linking).

::: js/src/asmjs/WasmCompile.cpp
@@ +723,5 @@
>      return init->tables.append(table);
>  }
>  
>  static bool
> +DecodeGlobalMetadata(Decoder& d, ValType* type, bool* isMutable)

How about DecodeGlobalType (given that type and mutability are both part of the "type").  Rossberg might even be proposing later that mutability be a compound type (so the "type" would be (mutable (i32)).

@@ +743,5 @@
> +    return true;
> +}
> +
> +static bool
> +GlobalIsJSCompatible(Decoder& d, ValType type, bool isMutable)

How about "ExternalGlobalIsJSCompatible" (where "External" = Imported or Exported)?

@@ +994,5 @@
> +      case Expr::I64Const: {
> +        int64_t i64;
> +        if (!d.readVarS64(&i64))
> +            return Fail(d, "failed to read initializer i64 expression");
> +        *val= Val(uint64_t(i64));

nit: space before '='

@@ +1019,5 @@
> +        if (i >= globals.length())
> +            return Fail(d, "global index out of range in initializer expression");
> +        if (!globals[i].isConstant())
> +            return Fail(d, "get_global initializer expression must reference a constant");
> +        *val = globals[i].constantValue();

I'm afraid this part needs to be more complicated: the requirement is that the global be an immutable *import*.  That means we need to record the index and assign the value at instantiation-time.  I expect you'll need a new InitExpr type in WasmTypes.h that is a union between a Val and a global-index and that is stored in GlobalDesc.

::: js/src/asmjs/WasmInstance.cpp
@@ +348,5 @@
> +          case GlobalKind::Variable:
> +            global.initialValue().writePayload(globalData + global.offset());
> +            break;
> +          case GlobalKind::Constant:
> +            continue;

So, IIUC, *technically* GlobalKind::Constant is entirely superfluous after compilation inside Metadata; it's mostly a convenience in that it allows you to (1) simply Move() from shared_->globals to metadata_->globals, (2) not have to re-index the global-indices in Exports.  That seems like a fine tradeoff for now, but if you imagine that people use non-imported constant globals as "constant pools", then perhaps we'll want to maintain a second index into the deflated non-Constant GlobalDesc array.

::: js/src/asmjs/WasmJS.cpp
@@ +23,5 @@
>  #include "asmjs/WasmModule.h"
>  
>  #include "jsobjinlines.h"
>  
> +#include "jit/JitOptions.h"

I think this needs to go right under the WasmModule.h #include (no \n) or else 'make check-style' will complain.

::: js/src/asmjs/WasmModule.cpp
@@ +21,5 @@
>  #include "asmjs/WasmInstance.h"
>  #include "asmjs/WasmJS.h"
>  #include "asmjs/WasmSerialize.h"
>  
> +#include "jit/JitOptions.h"

I think no \n before this #include

@@ +528,5 @@
> +                  const ValVector& globalImports, MutableHandleValue jsval)
> +{
> +    const GlobalDesc& global = globals[globalIndex];
> +
> +    MOZ_ASSERT(!global.isMutable(), "mutable variables can't be exported");

Since only Variables can be exported, it seems like the MOZ_CRASH below subsumes this.

@@ +552,5 @@
> +        jsval.set(ObjectValue(*obj));
> +        return true;
> +      }
> +      case ValType::F32: {
> +        jsval.set(NumberValue(double(value.f32())));

I actually think we should use DoubleValue() here and below.  Why?  NumberValue() tries to represent doubles as ints (when there is no fractional part) and so it can produce type instability in a JIT consumer where, unlike JS, we have a pretty strong hint here from wasm that there is likely going to be fractional part some time in the future.

@@ +638,5 @@
>  
>  bool
>  Module::instantiate(JSContext* cx,
>                      Handle<FunctionVector> funcImports,
> +                    const ValVector& globalImports,

For regularity in ordering (with other places where we list these out, could you put globalImports after memoryImport?

::: js/src/asmjs/WasmModule.h
@@ +129,5 @@
>      } pod;
>  
>    public:
>      Export() = default;
> +    explicit Export(UniqueChars fieldName, uint32_t funcExportIndex, DefinitionKind kind);

s/funcExportIndex/index/

::: js/src/asmjs/WasmTypes.h
@@ +413,5 @@
>      static bool match(const Sig* lhs, Lookup rhs) { return *lhs == rhs; }
>  };
>  
> +// A GlobalDesc describes a single global variable. Currently, asm.js and wasm
> +// exposes mutable and immutable private globals, but wasm can't import nor

asm.js can't import mutable globals either, so perhaps remove "wasm" in "but wasm can't".

@@ +423,5 @@
> +    Constant,
> +    Variable
> +};
> +
> +class GlobalDesc

I really like what you've done here.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.global-section-impl.patch

Duh, I should've read the spec more closely! This implements the right behavior for initializer expressions wrt get_global of immutable imports, which can then be reused in the patch 6 for elem segments.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.text.patch

Updated the text format counterparts (only tests have changed since the previous version).

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6.elem-initializers.patch

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7.asmjs-constant-pools.patch

Glad we have the asm.js tests, they helped find a few subtle issues.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8774443 [details] [diff] [review]
4.global-section-impl.patch

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

Ah, most excellent, only tiny nits:

::: js/src/asmjs/WasmGenerator.h
@@ +135,5 @@
>      MOZ_MUST_USE bool finishCodegen();
>      MOZ_MUST_USE bool finishLinkData(Bytes& code);
>      MOZ_MUST_USE bool addFuncImport(const Sig& sig, uint32_t globalDataOffset);
>      MOZ_MUST_USE bool allocateGlobalBytes(uint32_t bytes, uint32_t align, uint32_t* globalDataOff);
> +    MOZ_MUST_USE bool allocateGlobal(GlobalDesc& global);

Since it's mutated, can you take a GlobalDesc*?

@@ +164,5 @@
>      uint32_t numFuncSigs() const { return shared_->funcSigs.length(); }
>      const SigWithId& funcSig(uint32_t funcIndex) const;
>  
>      // Globals:
> +    MOZ_MUST_USE bool addGlobal(ValType type, bool isConst, uint32_t* index);

Can you move this below to be in the "asm.js" group?

::: js/src/asmjs/WasmInstance.cpp
@@ +402,5 @@
> +    for (size_t i = 0; i < metadata.globals.length(); i++) {
> +        const GlobalDesc& global = metadata.globals[i];
> +        switch (global.kind()) {
> +          case GlobalKind::Import: {
> +            globalImports[global.importIndex()].writePayload(globalData + global.offset());

How about hoisting the expression 'globalData + global.offset' out of the switch since it is used 3x?

::: js/src/asmjs/WasmJS.cpp
@@ +147,5 @@
> +
> +          case DefinitionKind::Global:
> +            Val val;
> +            const GlobalDesc& global = globals[globalIndex++];
> +            MOZ_ASSERT(global.isImport());

I believe you could replace this with an even stronger assert:
  MOZ_ASSERT(global.importIndex() == globalIndex - 1);
But, in that case, do we even need GlobalDesc::importIndex() since, in the one loop I see using it in Instance(), you could use the loop index instead?  Or am I confusing two index spaces here?

Also, can you MOZ_ASSERT(!global.isMutable())?

@@ +179,5 @@
> +                val = Val(d);
> +                break;
> +              }
> +              default: {
> +                return Throw(cx, "unexpected import value type");

Can this be a MOZ_CRASH() instead (given that this is a validated module)?

@@ +236,5 @@
>      Rooted<FunctionVector> funcs(cx, FunctionVector(cx));
>      RootedWasmTableObject table(cx);
>      RootedWasmMemoryObject memory(cx);
> +    ValVector globals;
> +    if (!GetImports(cx, *module, importObj, &funcs, &globals, &table, &memory))

Can you make 'globals' the last argument (to preserve the general (table,memory,globals) ordering pattern)?

::: js/src/asmjs/WasmModule.cpp
@@ +528,5 @@
> +{
> +    const GlobalDesc& global = globals[globalIndex];
> +
> +    // Imports are located upfront in the globals array.
> +    Val value;

Perhaps name 'val' for consistency with other Val variable names?

@@ +559,5 @@
> +      }
> +      default: {
> +        MOZ_CRASH("unexpected type when creating global exports");
> +      }
> +    }

I bet this function will get a compiler warning if you don't put a MOZ_CRASH on the last line.  Perhaps make the default case a 'break' then?

::: js/src/asmjs/WasmTypes.h
@@ +440,5 @@
> +  public:
> +    InitExpr() = default;
> +
> +    explicit InitExpr(Val val) : kind_(Kind::Constant)
> +    {

Can you put { on prev line to match { style in type()?  (Also below x1)

@@ +491,5 @@
> +            } val;
> +            unsigned offset;
> +            bool isMutable;
> +        } var;
> +        Val cst;

Can you give all the "leaf" fields trailing _?

Comment 28

[Luke Wagner [:luke]]

Comment on attachment 8774444 [details] [diff] [review]
5.text.patch

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

Very nice!

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +2462,5 @@
>      return true;
>  }
>  
> +static bool
> +ParseGlobalMetadata(WasmParseContext& c, WasmToken* typeToken, uint32_t* flags)

ParseGlobalType() to match WasmCompile change?

Comment 29

[Luke Wagner [:luke]]

Comment on attachment 8774445 [details] [diff] [review]
6.elem-initializers.patch

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

::: js/src/asmjs/WasmModule.cpp
@@ +427,5 @@
>  
>      // Now that all tables have been initialized, write elements.
> +    Uint32Vector prevEnds;
> +    if (!prevEnds.appendN(0, tables.length()))
> +        return false;

Uint32Vector uses SystemAllocPolicy which doesn't ReportOutOfMemory(); probably better to switch to `Vector<uint32_t> prevEnds(cx);` which will report for you.

::: js/src/asmjs/WasmModule.h
@@ +161,5 @@
>  
>  struct ElemSegment
>  {
>      uint32_t tableIndex;
> +    InitExpr initOffset;

I'd keep the name 'offset'; the type tells you it's an initializer expression.

Comment 30

[Luke Wagner [:luke]]

Comment on attachment 8774446 [details] [diff] [review]
7.asmjs-constant-pools.patch

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

::: js/src/asmjs/WasmGenerator.cpp
@@ +637,5 @@
>  {
>      MOZ_ASSERT(isAsmJS());
>      MOZ_ASSERT(!startedFuncDefs_);
>  
> +    GlobalDesc global(type, !isConst, *numImports++);

I worry this will cause assertions since we're adding mutable global imports.  When asm.js has:
  function f(stdlib, foreign) {
    "use asm";
    var x = foreign.y|0;
    ...
  }
that's not really a mutable import of 'y', rather, it's a mutable global 'x' whose init-expr is the immutable import 'y'.  A mutable import would involve some state shared with the outside world.

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review! All other remarks addressed, but...

(In reply to Luke Wagner [:luke] from comment #27)
> I believe you could replace this with an even stronger assert:
>   MOZ_ASSERT(global.importIndex() == globalIndex - 1);
> But, in that case, do we even need GlobalDesc::importIndex() since, in the
> one loop I see using it in Instance(), you could use the loop index instead?
> Or am I confusing two index spaces here?

importIndex is the index in the imported global values vector. In Instance(), we could simply use the loop index until we had InitExpr: a global X initializer can make use of another global import Y. To know which one, we need to know the index of Y in the imported global values vector (importIndex), which is different from the loop index of X and can't be inferred otherwise.

Comment 32

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #30)
> Comment on attachment 8774446 [details] [diff] [review]
> 7.asmjs-constant-pools.patch
> 
> Review of attachment 8774446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/WasmGenerator.cpp
> @@ +637,5 @@
> >  {
> >      MOZ_ASSERT(isAsmJS());
> >      MOZ_ASSERT(!startedFuncDefs_);
> >  
> > +    GlobalDesc global(type, !isConst, *numImports++);
> 
> I worry this will cause assertions since we're adding mutable global
> imports.  When asm.js has:
>   function f(stdlib, foreign) {
>     "use asm";
>     var x = foreign.y|0;
>     ...
>   }
> that's not really a mutable import of 'y', rather, it's a mutable global 'x'
> whose init-expr is the immutable import 'y'.  A mutable import would involve
> some state shared with the outside world.

That's a good point, although I think it's already an issue in the current state; before this last patch, all the AsmJS globals are considered to be imports, because it's simpler even for variables to pass the initial value at runtime, and for constants we just generate the constant inline in the wasm bytecode stream passed to WasmGenerator.

I'm fine to shelve this patch to think about it a bit more, until we get real mutable Global exports.

Comment 33

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7. asmjs-constant-pool.patch

Shelved for the moment.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d504768f3183
Rename {Load/Store}Global to {Get,Set}Global in wasm bytecode; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/45243283a2cd
Implement codegen for i64 globals on x64; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f1630b4ec6
Implement Global section in WebAssembly; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/524043583ed4
Implement text-to-binary for Globals; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f38091eaca2
Allow any expression in ElemSegments offset initializers; r=luke

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

Try push all green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c91fdf6e5539

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d504768f3183
https://hg.mozilla.org/mozilla-central/rev/45243283a2cd
https://hg.mozilla.org/mozilla-central/rev/78f1630b4ec6
https://hg.mozilla.org/mozilla-central/rev/524043583ed4
https://hg.mozilla.org/mozilla-central/rev/5f38091eaca2