1288222 - Baldr: change to structural signature match

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

... to match the recent change to call_indirect in AstSemantics.md#calls made so that cross-module table calls work as expected.

Comment 1

[Luke Wagner [:luke]]

Attachment: factor-global

Turns out we can share a bunch of code.  This avoids adding more #ifdef duplication in the next patch.

Comment 2

[Luke Wagner [:luke]]

Attachment: structural-sig

This patch solves the cross-module signature-unification problem as follows:
 - in the 99.7% (measured on AngryBots) case, the signature has a few number of arguments so the signature can be packed, by value, into a uint32 immediate that is passed and checked just like it is today (instead of the signature index), so no performance change from today
 - in the .3% case, the signature is big so we "atomize" the signature into a global HashMap and use the pointer as the value to compare on both sides.  (The atomized signature is ref-counted by all live dependent Instances.)

A new concept "signature id" is introduced which means "the immediate or the pointer-to-atomized-signature".  A new union type SigIdDesc is added which describes a sig-id by telling either the immediate u32 value or the offset in global-data to load the atomized signature from.  DeclaredSig is repurposed to hold both the Sig and SigIdDesc, so it is renamed SigWithId.  SigWithId must now be saved in metadata so that the instance can know what to atomize and where to put the result.

Btw, unrestricted unions in C++11 are the best; we can now throw off the tyranny of not being able to give constructors to types store din unions (see MAsmJSCall::Callee).

Comment 3

[Luke Wagner [:luke]]

Attachment: structural-sig

Oops, forgot to fill in comments.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8772985 [details] [diff] [review]
factor-global

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

So much better, would review again, 10/10, like, retweet, +1.

::: js/src/jit/CodeGenerator.cpp
@@ +11248,5 @@
> +void
> +CodeGenerator::visitAsmJSLoadFFIFunc(LAsmJSLoadFFIFunc* ins)
> +{
> +    MAsmJSLoadFFIFunc* mir = ins->mir();
> +    masm.loadWasmGlobalPtr(ToRegister(ins->output()), mir->globalDataOffset());

Feel free to inline ins->mir() here.

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ -2741,5 @@
> -                      wasm::JumpTarget::OutOfBounds);
> -    }
> -
> -    masm.ma_ldr(Address(GlobalReg, mir->globalDataOffset() - AsmJSGlobalRegBias), out);
> -    masm.ma_ldr(DTRAddr(out, DtrRegImmShift(index, LSL, 2)), out);

(The new code is so much more understandable than these two lines, namely the second one)

::: js/src/jit/mips-shared/CodeGenerator-mips-shared.cpp
@@ -2146,5 @@
> -                      wasm::JumpTarget::OutOfBounds);
> -    }
> -
> -    BaseIndex source(GlobalReg, index, ScalePointer, addr);
> -    masm.loadPtr(source, out);

Ha, so it seems MIPS could only do one loads where we now do two; it seems fine though.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
Actually, thinking about it and seeing usage in the following patch, it's uncommon in the generic masm to have the destination register on the left part of the signature of the masm function.

Could we instead have loadWasmGlobalPtr(uint32_t, Register), please?

Comment 6

[Luke Wagner [:luke]]

Righto!

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8773001 [details] [diff] [review]
structural-sig

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

Mostly nits. It's actually pretty mechanic and simple to understand. Thank you!

::: js/src/asmjs/WasmCode.cpp
@@ +249,5 @@
>      DeallocateExecutableMemory(bytes_, totalLength(), gc::SystemPageSize());
>  }
>  
> +size_t
> +Sig::serializedSize() const

As Sig is defined in WasmTypes.h, could you move these implementations to WasmTypes.cpp?

::: js/src/asmjs/WasmInstance.cpp
@@ +64,5 @@
> +
> +    bool allocateSigId(JSContext* cx, const Sig& sig, const void** sigId) {
> +        Map::AddPtr p = map_.lookupForAdd(sig);
> +        if (p) {
> +            p->value()++;

Can you MOZ_ASSERT(p->value()); above this line?

@@ +70,5 @@
> +            return true;
> +        }
> +
> +        UniquePtr<Sig> clone = MakeUnique<Sig>();
> +        if (!clone || !clone->clone(sig) || !map_.add(p, clone.get(), 1)) {

We could as well count from 0 (as having p->value() == 0 can't happen in practice), but all these nice assertions couldn't happen in that case, so OK with the current state.

@@ +92,5 @@
> +        }
> +    }
> +};
> +
> +ExclusiveData<SigIdSet> sigIdSet;

Just to be sure to understand: we need the lock because finalizing can happen off main thread, right? (and instantiation will always happen on the main thread)

@@ +456,5 @@
> +{
> +    if (!metadata_->sigIds.empty()) {
> +        ExclusiveData<SigIdSet>::Guard lockedSigIdSet = sigIdSet.lock();
> +
> +        if (!lockedSigIdSet.get().ensureInitialized(cx))

For what it's worth, ExclusiveData overrides conversion to T& and T* operators, so you could use -> here instead of get().

@@ +479,5 @@
>          if (exit.baselineScript)
>              exit.baselineScript->removeDependentWasmImport(*this, i);
>      }
> +
> +

nit: two blank lines

::: js/src/asmjs/WasmTypes.cpp
@@ +332,5 @@
> +typedef uint32_t ImmediateType;  // for 32/64 consistency
> +static const unsigned sImmediateBits = sizeof(ImmediateType) * 8 - 1;  // -1 for ImmediateBit
> +static const unsigned sBitsPerLength = 4;
> +static const unsigned sBitsPerType = 2;
> +static const unsigned sMaxTypes = (sImmediateBits - (2 * sBitsPerLength)) / sBitsPerType;

As discussed on IRC, the times 2 can go, here.

@@ +352,5 @@
> +
> +static ImmediateType
> +LengthToBits(uint32_t length)
> +{
> +    static_assert(sMaxTypes <= ((1 << sBitsPerLength) - 1), "fits");

MaxArgsPerFunc is a *bit* higher than that, we might need to lower it.

@@ +360,5 @@
> +
> +static ImmediateType
> +TypeToBits(ValType type)
> +{
> +    static_assert(3 <= ((1 << sBitsPerType) - 1), "fits");

So 3 is the last possible used value of uint32_t(ValType) - 1. Could we have a static const ValType LastSigValType (or a value in ValType being LastSigType = F64)?

What about SIMD indirect calls? asm.js might use those, I think? (I hope it is tested somewhere...)

::: js/src/asmjs/WasmTypes.h
@@ -428,5 @@
>  
> -// A "declared" signature is a Sig object that is created and owned by the
> -// ModuleGenerator. These signature objects are read-only and have the same
> -// lifetime as the ModuleGenerator. This type is useful since some uses of Sig
> -// need this extended lifetime and want to statically distinguish from the

Mentioning lifetime in the new comment still sounds useful to me.

::: js/src/jit/MIR.h
@@ +13467,4 @@
>        private:
>          Which which_;
> +        union U {
> +            U() {}

wow

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > +ExclusiveData<SigIdSet> sigIdSet;
> 
> Just to be sure to understand: we need the lock because finalizing can
> happen off main thread, right? (and instantiation will always happen on the
> main thread)

Workers :)  Technically, I could've put the sigIdSet on the JSRuntime (you have to opt-in to JSCLASS_BACKGROUND_FINALIZE and I haven't remembered to do that yet in WasmJS.cpp) but the eventual goal is sharing a Table between threads which needs whole-process unification.

> > +    static_assert(sMaxTypes <= ((1 << sBitsPerLength) - 1), "fits");
> 
> MaxArgsPerFunc is a *bit* higher than that, we might need to lower it.

Well, this is only an optimization; it's fine if the function has more arguments; it just doesn't get an immediate.

> What about SIMD indirect calls? asm.js might use those, I think? (I hope it
> is tested somewhere...)

Good catch; this is only used for wasm, but it'd be nice to be future proof.  I think the fix to this and your other comment is to have an IsImmediateType(ValType v) predicate that returns true if (v-1) is in the range [1, 1<<sTypeBits) and require IsImmediateType for all args/ret to fit in an immediate.

> > -// A "declared" signature is a Sig object that is created and owned by the
> > -// ModuleGenerator. These signature objects are read-only and have the same
> > -// lifetime as the ModuleGenerator. This type is useful since some uses of Sig
> > -// need this extended lifetime and want to statically distinguish from the
> 
> Mentioning lifetime in the new comment still sounds useful to me.

Well, unfortunately it's no longer strictly true because SigWithId objects are stored in Metadata as well.  So the lifetime is more a property of ModuleGeneratorData::sigs not being resized during compilation (which is a property of all the Vectors in ModuleGeneratorData).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e22104e1aa
Baldr: factor out common global access code (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55c09896ad7
Baldr: match signature types structurally (r=bbouvier)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32e22104e1aa
https://hg.mozilla.org/mozilla-central/rev/d55c09896ad7