Baldr: allow imported JS functions to be placed in tables

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Assignee)

Description

8 months ago
As recently added in
  https://github.com/WebAssembly/design/pull/817
Priority: -- → P2
(Assignee)

Updated

8 months ago
Priority: P2 → P1
(Assignee)

Comment 1

8 months ago
Created attachment 8806950 [details] [diff] [review]
maybe-start

Unrelated, but I brushed up against this and thought it'd be a bit nicer with a Maybe.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8806950 - Flags: review?(bbouvier)
(Assignee)

Comment 2

8 months ago
Created attachment 8806960 [details] [diff] [review]
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.
Attachment #8806960 - Flags: review?(bbouvier)
Comment on attachment 8806950 [details] [diff] [review]
maybe-start

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

Sweet
Attachment #8806950 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 4

8 months ago
Created attachment 8807369 [details] [diff] [review]
rm-func-def

Slightly updated to simplify subsequent patches.
Attachment #8806960 - Attachment is obsolete: true
Attachment #8806960 - Flags: review?(bbouvier)
Attachment #8807369 - Flags: review?(bbouvier)
(Assignee)

Comment 5

8 months ago
Created attachment 8807370 [details] [diff] [review]
tidy

This patch just moves a few things around.
Attachment #8807370 - Flags: review?(bbouvier)
(Assignee)

Comment 6

8 months ago
Created attachment 8807371 [details] [diff] [review]
rm-trap-offset

I realized this argument was dead (because the bad-indirect-call trap reports the bytecode offset of the caller).
Attachment #8807371 - Flags: review?(bbouvier)
(Assignee)

Comment 7

8 months ago
Created attachment 8807373 [details] [diff] [review]
js-in-tables

With the unified function index space, and just generating a wrapper function per import, this patch was surprisingly easy.
Attachment #8807373 - Flags: review?(bbouvier)
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)?
Attachment #8807369 - Flags: review?(bbouvier) → review+
Attachment #8807370 - Flags: review?(bbouvier) → review+
Comment on attachment 8807371 [details] [diff] [review]
rm-trap-offset

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

Indeed.
Attachment #8807371 - Flags: review?(bbouvier) → review+
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/
Attachment #8807373 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 11

8 months ago
Created attachment 8807670 [details] [diff] [review]
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.
Attachment #8807670 - Flags: review?(bbouvier)
Attachment #8807670 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 12

8 months ago
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.
(Assignee)

Comment 13

8 months ago
(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

8 months ago
Pushed by lwagner@mozilla.com:
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)
(Assignee)

Comment 15

8 months ago
Created attachment 8807782 [details] [diff] [review]
clean-up-f32
Attachment #8807782 - Flags: review?(bbouvier)
(Assignee)

Updated

8 months ago
Whiteboard: [leave open]

Comment 16

8 months ago
bugherder
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 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
(Assignee)

Comment 18

8 months ago
(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.
(Assignee)

Comment 19

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

Comment 20

8 months ago
Created attachment 8808345 [details] [diff] [review]
clean-up-f32

I also noticed you need >16 float arguments to hit stack spilling on ARM32, so the previous test is inflated.
Attachment #8807782 - Attachment is obsolete: true
Attachment #8807782 - Flags: review?(bbouvier)
Attachment #8808345 - Flags: review?(bbouvier)
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)
Attachment #8808345 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 22

8 months ago
(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

8 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dae0685c9d2
Baldr: only allocate 4 bytes for float32 on the stack (r=bbouvier)
(Assignee)

Updated

8 months ago
Whiteboard: [leave open]

Comment 24

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6dae0685c9d2
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(luke)
(Assignee)

Comment 26

8 months ago
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?
Flags: needinfo?(luke)
(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.
(Assignee)

Comment 28

8 months ago
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!
(Assignee)

Comment 29

8 months ago
Created attachment 8809672 [details] [diff] [review]
fix-thinko

(Requesting Dan because Benjamin is on holiday tomorrow and this clearly needs to be in 52 :)
Attachment #8809672 - Flags: review?(sunfish)
(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
Maybe the wasm pref is enabled on AWFY?
Flags: needinfo?(hv1989)
(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
Flags: needinfo?(hv1989)
(Assignee)

Comment 33

8 months ago
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?
(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 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.
Attachment #8809672 - Flags: review?(sunfish) → review+

Comment 36

8 months ago
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eafea39ebf39
Baldr: fix accidental disabling of asm.js when wasm is disabled (r=sunfish)

Comment 37

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eafea39ebf39

Updated

7 months ago
Depends on: 1317967

Updated

7 months ago
Depends on: 1317986
Depends on: 1318039
No longer depends on: 1318039
You need to log in before you can comment on or make changes to this bug.