1313180 - Baldr: allow imported JS functions to be placed in tables

Status: RESOLVED FIXED

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

Description

[Luke Wagner [:luke]]

As recently added in
  https://github.com/WebAssembly/design/pull/817

Comment 1

[Luke Wagner [:luke]]

Attachment: maybe-start

Unrelated, but I brushed up against this and thought it'd be a bit nicer with a Maybe.

Comment 2

[Luke Wagner [:luke]]

Attachment: rm-func-def

The next patch wants to use a lot more function-indices instead of function-definition-indices because all imports are now going to be given trivial wrapper functions (which gives you the entry stub, table-entry, profiling stuff, and everything for free!).  The main challenge is that the function index space starts with the imports and asm.js doesn't know the full set of imports until after validating every function body (since imports are built lazily based on (name, signature) usage).  After messing around with various strategies of mixing the two index spaces in various ways, I came to realize the simplest thing is to simply to kill the function-definition index space and use function-indices exclusively.  The only challenge is that asm.js doesn't know the total number of imports until the end of validation and the number of imports determines the function indices of function definitions.  To hack around this, asm.js sets a max number of imports and starts handing out indices for function definitions right after.  This ends up allowing simplifying a number of things.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8806950 [details] [diff] [review]
maybe-start

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

Sweet

Comment 4

[Luke Wagner [:luke]]

Attachment: rm-func-def

Slightly updated to simplify subsequent patches.

Comment 5

[Luke Wagner [:luke]]

Attachment: tidy

This patch just moves a few things around.

Comment 6

[Luke Wagner [:luke]]

Attachment: rm-trap-offset

I realized this argument was dead (because the bad-indirect-call trap reports the bytecode offset of the caller).

Comment 7

[Luke Wagner [:luke]]

Attachment: js-in-tables

With the unified function index space, and just generating a wrapper function per import, this patch was surprisingly easy.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8807369 [details] [diff] [review]
rm-func-def

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

Nice.

::: js/src/asmjs/AsmJS.cpp
@@ +2158,1 @@
>          if (funcIndex >= MaxFuncs)

This comparison should take AsmJSFirstDefFuncIndex into account; maybe init funcIndex to numFunctions() and add AsmJSFirstDefFuncIndex after the comparison?

@@ +2316,5 @@
>      Func* lookupFunction(PropertyName* name) {
>          if (GlobalMap::Ptr p = globalMap_.lookup(name)) {
>              Global* value = p->value();
>              if (value->which() == Global::Function)
> +                return functions_[value->funcIndex() - AsmJSFirstDefFuncIndex];

Can you MOZ_ASSERT(value->funcIndex() >= AsmJSFirstDefFuncIndex); before?

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +5359,5 @@
>  
>      sync();
>  
> +    const Sig& sig = *mg_.funcSigs[funcIndex];
> +    bool import = funcIndex < mg_.funcImportGlobalDataOffsets.length();

mg_.funcIsImport(funcIndex)?

::: js/src/asmjs/WasmGenerator.h
@@ +115,4 @@
>  
>      MOZ_MUST_USE bool finishOutstandingTask();
> +    bool isImport(uint32_t funcIndex) const;
> +    bool isCompiled(uint32_t funcIndex) const;

It's nice and short, but it could be strange to read at callsites. funcIsImport/funcIsCompiled? Or maybe isFuncImport/isFuncCompiled? (if not isCompiledFunc/isImportedFunc; whatever makes the most sense in English)

::: js/src/asmjs/WasmIonCompile.cpp
@@ +1912,5 @@
>      if (f.inDeadCode())
>          return true;
>  
> +    const Sig& sig = *f.mg().funcSigs[funcIndex];
> +    bool import = funcIndex < f.mg().funcImportGlobalDataOffsets.length();

f.mg().funcIsImport(funcIndex)?

::: js/src/asmjs/WasmModule.cpp
@@ +538,5 @@
>          if (!JS_DefineProperty(cx, segment, "kind", value, JSPROP_ENUMERATE))
>              return false;
>          if (p->isFunction()) {
> +            value.setNumber((uint32_t)p->funcIndex());
> +            if (!JS_DefineProperty(cx, segment, "funcIndex", value, JSPROP_ENUMERATE))

Please make sure that :mbx knows so that WasmExplorer and other tools don't silently break.

::: js/src/asmjs/WasmTypes.h
@@ +1394,5 @@
> +// immediately start handing out function indices starting at the maximum + 1.
> +// this means that there is a "hole" between the last import and the first
> +// definition, but that's fine.
> +
> +static const unsigned AsmJSMaxImports             = 4 * 1024;

Can you static_assert(AsmJSMaxImports <= MaxImports)?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8807371 [details] [diff] [review]
rm-trap-offset

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

Indeed.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8807373 [details] [diff] [review]
js-in-tables

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

\o/ \o/ \o/

::: js/src/asmjs/WasmGenerator.cpp
@@ +984,5 @@
> +            if (masm_.oom())
> +                return false;
> +
> +            uint32_t codeRangeIndex = metadata_->codeRanges.length();
> +            if (!metadata_->codeRanges.emplaceBack(funcIndex, 0, offsets))

Can you please add a comment before the 0: /* bytecodeOffset */

::: js/src/asmjs/WasmStubs.cpp
@@ +341,5 @@
> +#endif
> +    } else {
> +        MOZ_ASSERT(IsFloatingPointType(type));
> +        masm.loadDouble(src, ScratchDoubleReg);
> +        masm.storeDouble(ScratchDoubleReg, dst);

(pre-existing) strange that we use the double variants even for float32; although it works since the ABGIArgValIter uses an ABIArgGenerator under the hood, which allocates 64 bits for double *and* floats...

::: js/src/jit-test/tests/wasm/import-export.js
@@ +540,5 @@
> +new Instance(m, { "": { table: tbl, func: f} });
> +assertEq(tbl.get(0), null);
> +assertEq(tbl.get(1)(), 13);
> +assertEq(tbl.get(2)(), 42);
> +assertEq(tbl.get(3)(), undefined);

Can you throw in a few wasmFullPass tests please? (just to make sure re-exports are correctly handled by WasmBinaryToAST)

::: js/src/jit-test/tests/wasm/spec/linking.wast.js
@@ -1,3 @@
>  // |jit-test| test-also-wasm-baseline
> -// TODO type checks against exotic exported function objects
> -quit();

\o/

Comment 11

[Luke Wagner [:luke]]

Attachment: fix-msan

This fixes some issues that appeared with msan tests due to msan not working with jits and wasm not being properly disabled by --jittests=interp for msan runs.

Comment 12

[Luke Wagner [:luke]]

Note: Windows failures were caused by using ABIStackAlignment (=4, only on windows) instead of WasmStackAlignment (=16) which fails the JitStackAlignment (=16) test in the Ion call stub.

Comment 13

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
Great points, thanks!

> >          if (funcIndex >= MaxFuncs)
> 
> This comparison should take AsmJSFirstDefFuncIndex into account; maybe init
> funcIndex to numFunctions() and add AsmJSFirstDefFuncIndex after the
> comparison?

Well here we actually want MaxFuncs to be the max number of function indices so the check needs to happen after the addition.

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
Good points, thanks!

> (pre-existing) strange that we use the double variants even for float32;
> although it works since the ABGIArgValIter uses an ABIArgGenerator under the
> hood, which allocates 64 bits for double *and* floats...

You're right, that's really strange.  I was curious if it's always this way, and it seems it has:
  https://hg.mozilla.org/mozilla-central/rev/a63e23e9b03b#l2.1381
It's worth following up to see if this is really the right thing for native ABIs...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b561d60697
Baldr: turn off wasm tests for --jitflags=interp for SM-msan's benefit (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01106827c2b
Baldr: simplify startFuncIndex (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b3c5505c19
Baldr: switch everything to using function indices (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/700fccbb5044
Baldr: move around a few things (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a938e94dc04d
Baldr: remove unused TrapOffset arg (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7523af20b9
Baldr: allow JS imports in Tables (r=bbouvier)

Comment 15

[Luke Wagner [:luke]]

Attachment: clean-up-f32

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e9b561d60697
https://hg.mozilla.org/mozilla-central/rev/b01106827c2b
https://hg.mozilla.org/mozilla-central/rev/c3b3c5505c19
https://hg.mozilla.org/mozilla-central/rev/700fccbb5044
https://hg.mozilla.org/mozilla-central/rev/a938e94dc04d
https://hg.mozilla.org/mozilla-central/rev/ad7523af20b9

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8807782 [details] [diff] [review]
clean-up-f32

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

What about x64 and ARM which also use 64 bits for storing float32?

::: js/src/jit/x86/Assembler-x86.cpp
@@ +26,5 @@
>          stackOffset_ += sizeof(uint32_t);
>          break;
> +      case MIRType::Float32:
> +        current_ = ABIArg(stackOffset_);
> +        stackOffset_ += sizeof(uint32_t);

Could be commonized with the Int32/Pointer cases

Comment 18

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> What about x64 and ARM which also use 64 bits for storing float32?

Well they allocate 64-bit of space, but the extra 32 bits are padding meant to ensure subsequent word-sized args are word-aligned.

But actually, I see now that for ARM32 I was looking only at ABIArgGenerator::softNext() and hardNext() still adds a uint64_t.  I expect that is similarly wrong (and we just don't see it because we just don't call C++ passing float arguments), but I'll look into it.

Comment 19

[Luke Wagner [:luke]]

(FWIW, I checked and gcc does use only 4 bytes float arguments; I think we're just not actually calling C++ passing floats.)

Comment 20

[Luke Wagner [:luke]]

Attachment: clean-up-f32

I also noticed you need >16 float arguments to hit stack spilling on ARM32, so the previous test is inflated.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8808345 [details] [diff] [review]
clean-up-f32

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

Thanks, much better! (I assume you've tried-run this, because Ion now uses the ABIArgGen too, although I don't think that passing float32 arguments in ion->ion calls is allowed)

Comment 22

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #21)
> Thanks, much better! (I assume you've tried-run this, because Ion now uses
> the ABIArgGen too, although I don't think that passing float32 arguments in
> ion->ion calls is allowed)

Yeah, but I noticed we only run softfloat unless one fiddles with ARMHWCAP, but I did test this locally.  Actually, Ion uses ABIArgGen for making ABI calls (to C++); the Ion calling convention is all boxed and on the stack.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dae0685c9d2
Baldr: only allocate 4 bytes for float32 on the stack (r=bbouvier)

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dae0685c9d2

Comment 25

[Chris Peterson [:cpeterson]]

Luke, this change caused my Octane zlib score to drop from ~70,000 to ~33,000 on my Windows 10 machine (for both 32-bit and 64-bit Firefox). Is that expected?

Comment 26

[Luke Wagner [:luke]]

That's weird, I can't observe any such drop between DevEd and Nightly (which would cover this cset) nor do I see any change in browser or shell octane-zlib on awfy.  I do see a drop of that magnitude when I disable asm.js via javascript.options.asmjs; do you see any "asm.js is disabled" warnings?

Comment 27

[Chris Peterson [:cpeterson]]

(In reply to Luke Wagner [:luke] from comment #26)
> That's weird, I can't observe any such drop between DevEd and Nightly (which
> would cover this cset) nor do I see any change in browser or shell
> octane-zlib on awfy.  I do see a drop of that magnitude when I disable
> asm.js via javascript.options.asmjs; do you see any "asm.js is disabled"
> warnings?

Actually, yes:

TypeError: asm.js type error: Disabled by lack of compiler support. zlib-data.js
unreachable code after return statement. zlib-data.js:1:162549
...
unreachable code after return statement. zlib-data.js:1:162549
zlib: 33098

But my javascript.options.asmjs pref is true and I can reproduce this in clean profile. Is there any information about my system configuration that would be useful for determining why my machine "lacks compiler support"? I'm running the 64-bit Windows 10 Insider Preview channel, if that matters.

Comment 28

[Luke Wagner [:luke]]

Oh my goodness, I see the (stupid) bug:
  https://hg.mozilla.org/mozilla-central/rev/e9b561d60697#l12.10
this ends up requiring javascript.options.wasm=true for asm.js to be enabled.  (And of course I didn't notice b/c I end up having that option set in all my profiles and it's on by default in the shell.)  D'oh!  Thanks for finding!

Comment 29

[Luke Wagner [:luke]]

Attachment: fix-thinko

(Requesting Dan because Benjamin is on holiday tomorrow and this clearly needs to be in 52 :)

Comment 30

[Chris Peterson [:cpeterson]]

(In reply to Luke Wagner [:luke] from comment #28)
> Oh my goodness, I see the (stupid) bug:
>   https://hg.mozilla.org/mozilla-central/rev/e9b561d60697#l12.10
> this ends up requiring javascript.options.wasm=true for asm.js to be
> enabled.

Why didn't this zlib regression show up on AWFY?

https://arewefastyet.com/#machine=29&view=single&suite=octane&subtest=zlib

Comment 31

[Jan de Mooij [:jandem]]

Maybe the wasm pref is enabled on AWFY?

Comment 32

[Hannes Verschore [:h4writer]]

(In reply to Chris Peterson [:cpeterson] from comment #30)
> (In reply to Luke Wagner [:luke] from comment #28)
> > Oh my goodness, I see the (stupid) bug:
> >   https://hg.mozilla.org/mozilla-central/rev/e9b561d60697#l12.10
> > this ends up requiring javascript.options.wasm=true for asm.js to be
> > enabled.
> 
> Why didn't this zlib regression show up on AWFY?
> 
> https://arewefastyet.com/#machine=29&view=single&suite=octane&subtest=zlib

Awfy is running the shell. As a result it doesn't read the "javascript.options.wasm=true" about:config options.

If you look at the browser slaves you will see the regression:
https://arewefastyet.com/#machine=31&view=single&suite=octane&subtest=zlib

Comment 33

[Luke Wagner [:luke]]

Ah, I thought I had been looking at browser builds by clicking on "Win 8 32-bit (i7-4700HQ, browser)".  Is this a shell build by a browser build?

Comment 34

[Hannes Verschore [:h4writer]]

(In reply to Luke Wagner [:luke] from comment #33)
> Ah, I thought I had been looking at browser builds by clicking on "Win 8
> 32-bit (i7-4700HQ, browser)".  Is this a shell build by a browser build?

The Win 8 browser slave is a normal browser and is running, but I have been improving the software a bit to allow try builds etc soonish. Somehow creating the graphs for that browser failed as a result. I haven't found the time to investigate why. But hopefully soonish it should show the current state again.

Comment 35

[Dan Gohman [:sunfish]]

Comment on attachment 8809672 [details] [diff] [review]
fix-thinko

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

Ah, this changes some HasCompilerSupport calls to HasSupport, but leaves the ones in AsmJS.cpp in place, so it doesn't depend on the wasm pref.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eafea39ebf39
Baldr: fix accidental disabling of asm.js when wasm is disabled (r=sunfish)

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eafea39ebf39