1238679 - SIMD: Refactor representation of opcodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

The SIMD module is pretty big now. Nightly has 12 SIMD types, defining a total of 346 functions.

Code in IonMonkey and OdinMonkey gets quite repetitive when dealing with all these native functions. Ion is currently identifying these functions via function pointer comparisons in functions like IonBuilder::inlineSimdInt32x4(). This is not the fastest way of identifying native inlinable functions, and we would get many pages of repetitive, error prone code as we add Ion support for all the SIMD types.

The set of 346 SIMD functions has a lot of regularity since most operations are defined on multiple types. Grouping SIMD operations by name alone, there are 57 unique operation names.

Here's one way of dealing with all the repetition in the SIMD API:

Add an enum for the 57 unique SIMD operations. We already have SimdTypeDescr::Type enumerating the 12 types.

Provide a function for generating a SIMD opcode and embedding it in a larger opcode space:

  encodeSIMDop(BaseOpcode, SIMDType, SIMDOperation) -> Opcode

Provide a reverse mapping:

  decodeSIMDOpcode(BaseOpcode, SIMDOpcode) -> (SIMDType, SIMDOpcode)

Here, BaseOpcode indicates the beginning of a contiguous range in the opcode space that is reserved for SIMD operations. The opcode space is for example the js:jit::InlinableNative enum which also has other opcodes unrelated to SIMD.

These functions can be implemented as computations instead of giant switches if we allow some sparsity in the opcode space. A simple matrix mapping would need 12 x 57 = 684 opcodes to represent the 346 native SIMD functions, for about 50% sparsity.

Note that such a sparse encoding would be able to represent invalid SIMD operations like Int32x4.sqrt or Float32x4.addSaturate, but that is not a problem because invalid operations are filtered out before opcodes are encoded.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add enum class SIMDOperation.

This enumeration contains all SIMD operations that have a function name plus
the constructor.

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Provide per-operation JSJitInfo structs for the SIMD functions.

The InlinableNative enumeration doesn't have an enumerator for every SIMD
function, it only enumerates the SIMD types supported by the jit, and Ion uses
the JSNative function pointer to identify functions when inlining.

Reuse the uint16_t .depth field in JSJitInfo as a sub-opcode, and give each
inlinable SIMD operation its own JSJitInfo with a sub-opcode that identifies the
operation.

Use the old JSJitInfo SIMD structs from MCallOptimize.cpp to represent the
constructor calls. They all have .depth = 0 which corresponds to
SIMDOperation::Constructor.

This will make it easier to identify inlinable SIMD functions in Ion.

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Mark the Bool32x4 functions as inlinable.

This is an oversight from Bug 1160971.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Make inlineSimdLoad/Store take a MIRType argument.

Complete the earlier refactoring that got rid of the SimdTypeDescr::Type enums.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Implement main SIMD inlining dispatch.

Use a shared inlineSimd() dispatch function for all the SIMD types supported by
Ion. This function can dispatch on the subOpcoe stored in the JSJitInfo struct,
and we avoid comparing native function pointers that way.

Delete the old per-type dispatch functions.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

So it turns out that the sparse matrix mapping of the SIMD opcodes is not so easy to pull off for the InlinableNative enumeration. The JSFunctionSpec struct doesn't contain an InlinableNative enum; it only has a pointer to a JSJitInfo struct. The only way we can decode SIMD functions (other than comparing native function pointers) is to give each function its own JSJitInfo struct.

We can then reuse JSJitInfo.depth as a sub-opcode field, and encode things as inlinableNative -> type, .depth -> operation.

Notice in particular how much boilerplate code this saves as we add more SIMD types to Ion.

FWIW, I still think OdinMonkey should encode SIMD operations sparsely in the opcode space.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8706694 [details] [diff] [review]
Implement main SIMD inlining dispatch.

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

I like it. I'd also assert in every inline* function that the type is one of the possible types of the operations (e.g. that inlineSimdBinary(div) doesn't receive int32x4 operands).

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8706693 [details] [diff] [review]
Make inlineSimdLoad/Store take a MIRType argument.

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

Nice.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8706691 [details] [diff] [review]
Provide per-operation JSJitInfo structs for the SIMD functions.

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

::: js/src/jsfriendapi.h
@@ +2340,1 @@
>      uint16_t depth;

How about using an union here? So that we can use a more meaningful name for the operation opcode.
And we can comment the usage of each field.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8706690 [details] [diff] [review]
Add enum class SIMDOperation.

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

::: js/src/builtin/SIMD.h
@@ +783,5 @@
> +//
> +// No SIMD types implement all of these operations.
> +//
> +// C++ defines keywords and/or/xor/not, so prepend Fn_ to all function names to avoid clashes.
> +enum class SIMDOperation : uint8_t {

The convention was to write "Simd" when it is a prefix, "SIMD" when it's in comments (to avoid things like AsmJSSIMDType where most letters are uppercase and it feels like the variable's name is shouting at the reader).

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Looks nice, at first glance. Having the same sparse encoding in Odin sounds good as well, working on that.

Comment 12

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Comment on attachment 8706691 [details] [diff] [review]
> Provide per-operation JSJitInfo structs for the SIMD functions.
> 
> Review of attachment 8706691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsfriendapi.h
> @@ +2340,1 @@
> >      uint16_t depth;
> 
> How about using an union here? So that we can use a more meaningful name for
> the operation opcode.
> And we can comment the usage of each field.

I wasn't sure if this struct is part of the external API, so I didn't want to change it. (I couldn't find any documentation for the concept of "friend API".)

I initially put an extra field in the first union containing the getter/setter function pointers. I used a designated initializer like this

    const JSJitInfo JitInfo_Simd... = { { .subOpcode = SIMDOperation::foo }, ... };

This works great when compiling with Clang and GCC, but the MSVC compiler we use for our Windows builds doesn't support designated initializers. (They were standardized in 1999.)

Without designated initializers, union initializers always apply to the first member. For example, we initialize the first member of this union, even if we want to use the inlinableNative member:

    union {
        uint16_t protoID;
        js::jit::InlinableNative inlinableNative;
    };

Initializing one member of a union and reading another is technically undefined behavior, although this one is unlikely to cause any breakage. Casting between an enum and a function pointer seems more likely to break in the future, though.

This also seems risky:

    union {
        uint16_t depth;
        SIMDOperation simdOp;
    };

Since the enum is defined as "enum class SIMDOperation : uint8_t". It seems likely to break on a big endian architecture, and depending on SIMDOperation being declared as ": uint16_t" just so it can live in a union with a uint16_t seems fragile and wrong.

So this is the viable alternative:

    union {
        uint16_t depth;
        uint16_t simdOp;
    };

This requires all the initializers to list depth as "{0}" instead of "0", which is fine by me as long as all instances are confined to the Gecko tree.

Comment 13

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> Comment on attachment 8706690 [details] [diff] [review]
> Add enum class SIMDOperation.
> 
> Review of attachment 8706690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/SIMD.h
> @@ +783,5 @@
> > +//
> > +// No SIMD types implement all of these operations.
> > +//
> > +// C++ defines keywords and/or/xor/not, so prepend Fn_ to all function names to avoid clashes.
> > +enum class SIMDOperation : uint8_t {
> 
> The convention was to write "Simd" when it is a prefix, "SIMD" when it's in
> comments (to avoid things like AsmJSSIMDType where most letters are
> uppercase and it feels like the variable's name is shouting at the reader).

I was following the convention of "class SIMDObject" right below, But you are right, "Simd" is better.

I'll change both.

Comment 14

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Rename s/SIMD/Simd/ in type and function names.

We prefer to camelcase acronyms to avoid shouty identifiers. Most types and
functions with SIMD in their name already spell it 'Simd'.

Comment 15

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Put JSJitInfo::depth in anonymous union.

Add a JSJitInfo::nativeOp field which will be used to identify SIMD operations.

Comment 16

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add enum class SIMDOperation.

This enumeration contains all SIMD operations that have a function name plus
the constructor.

Comment 17

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Provide per-operation JSJitInfo for the SIMD functions.

The InlinableNative enumeration doesn't have an enumerator for every SIMD
function, it only enumerates the SIMD types supported by the jit, and Ion uses
the JSNative function pointer to identify functions when inlining.

Use the uint16_t .nativeOp field in JSJitInfo as a sub-opcode, and give each
inlinable SIMD operation its own JSJitInfo with a sub-opcode that identifies the
operation.

Use the old JSJitInfo SIMD structs from MCallOptimize.cpp to represent the
constructor calls. They all have .nativeOp = 0 which corresponds to
SimdOperation::Constructor.

This will make it easier to identify inlinable SIMD functions in Ion.

Comment 18

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Implement main SIMD inlining dispatch.

Use a shared inlineSimd() dispatch function for all the SIMD types supported by
Ion. This function can dispatch on the subOpcoe stored in the JSJitInfo struct,
and we avoid comparing native function pointers that way.

Delete the old per-type dispatch functions.

Comment 19

[Jakob Stoklund Olesen [:jolesen]]

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

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8708069 [details] [diff] [review]
Put JSJitInfo::depth in anonymous union.

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

This sounds good to me.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8708068 [details] [diff] [review]
Rename s/SIMD/Simd/ in type and function names.

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

Thanks!

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8708070 [details] [diff] [review]
Add enum class SIMDOperation.

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

Looks good! Can go as is, suggested a few things to make things simpler, but nothing absolutely necessary.

::: js/src/builtin/SIMD.h
@@ +782,5 @@
> +// Complete set of SIMD operations.
> +//
> +// No SIMD types implement all of these operations.
> +//
> +// C++ defines keywords and/or/xor/not, so prepend Fn_ to all named functions to

Feel the Fn_ prefix is a bit long, I would go with _ or such, but anyways text editors are good at autocompletion nowadays.

@@ +807,5 @@
> +    Fn_fromUint16x8Bits,
> +    Fn_fromUint32x4Bits,
> +    Fn_fromFloat32x4Bits,
> +    Fn_fromFloat64x2Bits,
> +#define SIMD_OPERATION_COUNT (1u + unsigned(SIMDOperation::Fn_fromFloat64x2Bits))

If you just put a value "Last" or "Count" at the end, 1. we won't have to worry about fromFloat64x2Bits being the last one on this list, 2. we'll be able to take advantage of mfbt::EnumeratedRange and all (not saying we will, but we might). Not a big deal, so it's fine to let as is.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8708072 [details] [diff] [review]
Provide per-operation JSJitInfo for the SIMD functions.

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

Looks good, cheers!

::: js/src/builtin/SIMD.cpp
@@ +199,5 @@
> +// named functions here. The default JitInfo_SimdInt32x4 etc structs represent the
> +// SimdOperation::Constructor.
> +#define DEFN(TYPE, OP) const JSJitInfo JitInfo_Simd##TYPE##_##OP = {    \
> +    { nullptr }, { uint16_t(InlinableNative::Simd##TYPE) },             \
> +    { uint16_t(SimdOperation::Fn_##OP) }, JSJitInfo::InlinableNative };

Can you please put one field per line, and inline-comment what each field does? It's a bit hard to read.

Also, will other fields of the JSJitInfo struct be implicitly set to their default values (0) here? I'd rather have them explicitly set.
Or is it that they don't matter, because they are ignored in MCallOptimize? It seems so. If that's the case, then just feel free to not explicit them and maybe add a comment instead, explaining why we don't set them?

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8708073 [details] [diff] [review]
Implement main SIMD inlining dispatch.

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

nit in the changeset message: subOpcoe => subOpcode.

Very nice, thank you very much!

::: js/src/jit/MCallOptimize.cpp
@@ +202,5 @@
>          return inlineObjectCreate(callInfo);
>  
>        // SIMD natives.
>        case InlinableNative::SimdInt32x4:
> +        return inlineSimd(callInfo, target->native(),

inlineSimd could just take the callInfo, target and MIRType as parameters and deduce what it wants from there. This would avoid the SimdOperation(target->jitInfo()->nativeOp) duplication.

@@ +3064,4 @@
>  
> +    switch(simdOp) {
> +      case SimdOperation::Constructor:
> +        MOZ_CRASH("SIMD constructor call not expected.");

Can you refer to inlineConstructSimdObject in the message or in an inline comment, please?

Comment 25

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #22)
> Comment on attachment 8708070 [details] [diff] [review]
> Add enum class SIMDOperation.

> > +// Complete set of SIMD operations.
> > +//
> > +// No SIMD types implement all of these operations.
> > +//
> > +// C++ defines keywords and/or/xor/not, so prepend Fn_ to all named functions to
> 
> Feel the Fn_ prefix is a bit long, I would go with _ or such, but anyways
> text editors are good at autocompletion nowadays.

I wanted the Fn_ prefix to clearly indicate named functions on the SIMD constructors: SIMD.Int32x4.add -> SimdOperation::Fn_add. This separates the constructor call SIMD.Int32x4(...) -> SimdOperation::Constructor. This is also anticipating other SIMD operations that maybe not have a constructor property equivalent. Such operations would not get a Fn_ prefix.

I also think the dangling _ is ugly ;-)

> @@ +807,5 @@
> > +    Fn_fromUint16x8Bits,
> > +    Fn_fromUint32x4Bits,
> > +    Fn_fromFloat32x4Bits,
> > +    Fn_fromFloat64x2Bits,
> > +#define SIMD_OPERATION_COUNT (1u + unsigned(SIMDOperation::Fn_fromFloat64x2Bits))
> 
> If you just put a value "Last" or "Count" at the end, 1. we won't have to
> worry about fromFloat64x2Bits being the last one on this list, 2. we'll be
> able to take advantage of mfbt::EnumeratedRange and all (not saying we will,
> but we might). Not a big deal, so it's fine to let as is.

Turns out I didn't need SIMD_OPERATION_COUNT, so I just removed it.

The problem with a sentinel 'Count' enumerator is that you have to include it in switches if you want compiler warnings for missing enumerators, and since this is an enum class, you would have to cast the sentinel to an integer type explicitly to get the number of operations: unsigned(SimdOperation::Count).

Comment 26

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #23)
> Comment on attachment 8708072 [details] [diff] [review]
> Provide per-operation JSJitInfo for the SIMD functions.
> 
> Review of attachment 8708072 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, cheers!
> 
> ::: js/src/builtin/SIMD.cpp
> @@ +199,5 @@
> > +// named functions here. The default JitInfo_SimdInt32x4 etc structs represent the
> > +// SimdOperation::Constructor.
> > +#define DEFN(TYPE, OP) const JSJitInfo JitInfo_Simd##TYPE##_##OP = {    \
> > +    { nullptr }, { uint16_t(InlinableNative::Simd##TYPE) },             \
> > +    { uint16_t(SimdOperation::Fn_##OP) }, JSJitInfo::InlinableNative };
> 
> Can you please put one field per line, and inline-comment what each field
> does? It's a bit hard to read.

Sure, good idea.

> Also, will other fields of the JSJitInfo struct be implicitly set to their
> default values (0) here? I'd rather have them explicitly set.
> Or is it that they don't matter, because they are ignored in MCallOptimize?
> It seems so. If that's the case, then just feel free to not explicit them
> and maybe add a comment instead, explaining why we don't set them?

As soon as you have an {...} initializer, all fields will be initialized whether you provide a value or not. Compilers will warn if -Wmissing-field-initializers is enabled and some values are missing, but it appears we don't enable that warning. See the other JSJitInfo constants at the end of MCallOptimize.cpp.

In this case, the remaining bitfields are not used for inlinable native functions, so I'll just add a comment to that effect.

Comment 27

[Jakob Stoklund Olesen [:jolesen]]

Thanks, for reviewing this, Ben!

Try run for updated patch series: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58fbd144f7b

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db7239a51eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/af12e3e733d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a2bda1e531
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b183a8cf314
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c1a62a453c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d469a2a37f
https://hg.mozilla.org/integration/mozilla-inbound/rev/02827acc616d

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0db7239a51eb
https://hg.mozilla.org/mozilla-central/rev/af12e3e733d1
https://hg.mozilla.org/mozilla-central/rev/f3a2bda1e531
https://hg.mozilla.org/mozilla-central/rev/4b183a8cf314
https://hg.mozilla.org/mozilla-central/rev/01c1a62a453c
https://hg.mozilla.org/mozilla-central/rev/a5d469a2a37f
https://hg.mozilla.org/mozilla-central/rev/02827acc616d