Closed Bug 1241454 Opened 4 years ago Closed 4 years ago

wasm: make SIMD opcodes simpler

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(4 files)

And let's get rid of a few redundant types.
Blocks: wasm
This hoists SimdType out of TypedObject.h :: SimdTypeDescr::Type, so that it's defined in SIMD.h, next to SimdOperation.

Then this merges AsmJSSimdType and SimdType, because DRY.
Attachment #8710372 - Flags: review?(jolesen)
This merges AsmJSSimdOperation and SimdOperation. To avoid a naming clash in AsmJSGlobal / Global, the enum value SimdOperation is renamed into SimdOp.
Attachment #8710376 - Flags: review?(jolesen)
Comment on attachment 8710372 [details] [diff] [review]
1. Unify AsmJSSimdType and SimdType

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

This is great!

::: js/src/asmjs/AsmJS.cpp
@@ +108,2 @@
>  static inline bool
> +IsSignedIntSimdType(SimdType type)

How about moving this function into SIMD.h? It seems generally useful.

@@ +2427,3 @@
>              if (!IsLiteralInt(m, arg, &_))
>                  return false;
> +            break;

Is this change intentional?

@@ +7327,5 @@
>      return true;
>  }
>  
>  static PropertyName*
> +SimdTypeToName(JSContext* cx, SimdType type)

This function could be generally useful. Should it live in SIMD.h / SIMD.cpp?

::: js/src/builtin/SIMD.cpp
@@ +426,1 @@
>      typeDescr->initReservedSlot(JS_DESCR_SLOT_KIND, Int32Value(type::Simd));

Not your change, but this is unfortunate: We declare a variable called ``type`` and then immediately refer to the namespace ``type::``. I was confused for a bit.

Perhaps rename (or delete) the variable, or explicitly refer to ``js::type::Simd``?

::: js/src/builtin/SIMD.h
@@ +792,5 @@
> +    Bool8x16  = JS_SIMDTYPEREPR_BOOL8X16,
> +    Bool16x8  = JS_SIMDTYPEREPR_BOOL16X8,
> +    Bool32x4  = JS_SIMDTYPEREPR_BOOL32X4,
> +    Bool64x2  = JS_SIMDTYPEREPR_BOOL64X2,
> +    Count

This is great!

There is a comment by the JS_SIMDTYPEREPR_* definitions in TypedObjectConstants.h that needs to be updated.

Could you also add a pointer to this enumeration in SIMD.h saying that it must be kept in sync, and that the numbering is significant for SimdType::Count to work as expected?

If you are extra paranoid, stick a static_assert(unsigned(SimdType::Count) == 12) in SIMD.cpp somewhere.

::: js/src/jit/CodeGenerator.cpp
@@ +4950,5 @@
>      JitCompartment* jitCompartment = cx->compartment()->jitCompartment();
>      while (simdRefreshTemplatesDuringLink_) {
>          uint32_t typeIndex = mozilla::CountTrailingZeroes32(simdRefreshTemplatesDuringLink_);
>          simdRefreshTemplatesDuringLink_ ^= 1 << typeIndex;
> +        SimdType type = SimdType(typeIndex);

Maybe add a range assertion here? Or will registerSimdTemplateObjectFor() do that anyway?

::: js/src/jit/Ion.cpp
@@ +752,5 @@
>  
>      if (regExpTesterStub_ && !IsMarkedUnbarriered(&regExpTesterStub_))
>          regExpTesterStub_ = nullptr;
>  
> +    for (ReadBarrieredObject& obj : simdTemplateObjects_) {

Much better!

::: js/src/jit/MCallOptimize.cpp
@@ +3253,5 @@
> +{
> +    switch (type) {
> +      case SimdType::Float32x4:   *mirType = MIRType_Float32x4; return true;
> +      case SimdType::Int32x4:     *mirType = MIRType_Int32x4;   return true;
> +      case SimdType::Bool32x4:    *mirType = MIRType_Bool32x4;  return true;

Nit: I try to use the order Int-Float-Bool in all the switches.

::: js/src/jit/MIR.h
@@ +69,5 @@
>  
> +static inline
> +SimdType MIRTypeToSimdType(MIRType type)
> +{
> +    MOZ_ASSERT(IsSimdType(type));

It's harmless, but this assertion is redundant.

::: js/src/jit/Recover.cpp
@@ +1334,5 @@
>  MSimdBox::writeRecoverData(CompactBufferWriter& writer) const
>  {
>      MOZ_ASSERT(canRecoverOnBailout());
>      writer.writeUnsigned(uint32_t(RInstruction::Recover_SimdBox));
> +    static_assert(sizeof(SimdType) == sizeof(uint8_t), "assuming uint8 storage class for SimdType");

Nice!

::: js/src/jit/TypedObjectPrediction.cpp
@@ -185,5 @@
>        case TypedObjectPrediction::Inconsistent:
>          break;
>  
>        case TypedObjectPrediction::Descr:
> -        return descr().as<T>().type();

I don't think this is right. This code is also used for extractType<ScalarTypeDescr> and extractType<ReferenceTypeDescr>().

Just leave this as is, and your change to simdType() below is all we need. It was the only place instantiating extractType<SimdTypeDescr>() AFAICT.

::: js/src/vm/GlobalObject.h
@@ +448,5 @@
>      getOrCreateSimdTypeDescr(JSContext* cx, Handle<GlobalObject*> global) {
>          RootedObject globalSimdObject(cx, global->getOrCreateSimdGlobalObject(cx));
>          if (!globalSimdObject)
>              return nullptr;
> +        uint32_t typeSlotIndex = unsigned(T::type);

This is academic, but why not ``uint32_t(T::type)``?
Attachment #8710372 - Flags: review?(jolesen) → review+
Comment on attachment 8710376 [details] [diff] [review]
2. Unify AsmJSSimdOperation and SimdOperation

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

Looks good!

::: js/src/asmjs/AsmJS.cpp
@@ +1443,5 @@
>              ArrayViewCtor,
>              MathBuiltinFunction,
>              AtomicsBuiltinFunction,
>              SimdCtor,
> +            SimdOp

Since there is a SimdOperation::Constructor opcode, you could probably merge SimdCtor into SimdOp here.

It is more appropriate for a follow-on patch, but it could save some code.
Attachment #8710376 - Flags: review?(jolesen) → review+
> ::: js/src/asmjs/AsmJS.cpp
> @@ +108,2 @@
> >  static inline bool
> > +IsSignedIntSimdType(SimdType type)
> 
> How about moving this function into SIMD.h? It seems generally useful.

Alternatively, if we decide to have a SimdSign enum class:

    static inline SimdSign
    SimdTypeSignedness(SimdType type) {
        Int..   => SimdType::Signed
        Uint... => SimdSign::Unsigned
        others  => SimdSign::NA
    }
Thank you for the quick reviews!

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #3)
> Comment on attachment 8710372 [details] [diff] [review]
> 1. Unify AsmJSSimdType and SimdType
> 
> Review of attachment 8710372 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: js/src/asmjs/AsmJS.cpp
> @@ +108,2 @@
> >  static inline bool
> > +IsSignedIntSimdType(SimdType type)
> 
> How about moving this function into SIMD.h? It seems generally useful.

Done, I've kept it that way so far; when we'll need NA, we can add it.

> 
> @@ +2427,3 @@
> >              if (!IsLiteralInt(m, arg, &_))
> >                  return false;
> > +            break;
> 
> Is this change intentional?

Yes it is. It wasn't supposed to fallthrough here, and it only worked because literal int => non float numeric literal, but this was a redundant check.
> 
> @@ +7327,5 @@
> >      return true;
> >  }
> >  
> >  static PropertyName*
> > +SimdTypeToName(JSContext* cx, SimdType type)
> 
> This function could be generally useful. Should it live in SIMD.h / SIMD.cpp?

Yes, moved it there.

> 
> ::: js/src/builtin/SIMD.cpp
> @@ +426,1 @@
> >      typeDescr->initReservedSlot(JS_DESCR_SLOT_KIND, Int32Value(type::Simd));
> 
> Not your change, but this is unfortunate: We declare a variable called
> ``type`` and then immediately refer to the namespace ``type::``. I was
> confused for a bit.
> 
> Perhaps rename (or delete) the variable, or explicitly refer to
> ``js::type::Simd``?

Indeed; renamed type to simdType.

> 
> ::: js/src/builtin/SIMD.h
> @@ +792,5 @@
> > +    Bool8x16  = JS_SIMDTYPEREPR_BOOL8X16,
> > +    Bool16x8  = JS_SIMDTYPEREPR_BOOL16X8,
> > +    Bool32x4  = JS_SIMDTYPEREPR_BOOL32X4,
> > +    Bool64x2  = JS_SIMDTYPEREPR_BOOL64X2,
> > +    Count
> 
> This is great!
> 
> There is a comment by the JS_SIMDTYPEREPR_* definitions in
> TypedObjectConstants.h that needs to be updated.
> 
> Could you also add a pointer to this enumeration in SIMD.h saying that it
> must be kept in sync, and that the numbering is significant for
> SimdType::Count to work as expected?
> 
> If you are extra paranoid, stick a static_assert(unsigned(SimdType::Count)
> == 12) in SIMD.cpp somewhere.

I like paranoid checks, I've added it at the top of SIMD.cpp, thanks.

> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +4950,5 @@
> >      JitCompartment* jitCompartment = cx->compartment()->jitCompartment();
> >      while (simdRefreshTemplatesDuringLink_) {
> >          uint32_t typeIndex = mozilla::CountTrailingZeroes32(simdRefreshTemplatesDuringLink_);
> >          simdRefreshTemplatesDuringLink_ ^= 1 << typeIndex;
> > +        SimdType type = SimdType(typeIndex);
> 
> Maybe add a range assertion here? Or will registerSimdTemplateObjectFor() do
> that anyway?

registerSimdTemplateObjectFor accesses the EnumeratedArray which does the range assertion for us.

> ::: js/src/jit/TypedObjectPrediction.cpp
> @@ -185,5 @@
> >        case TypedObjectPrediction::Inconsistent:
> >          break;
> >  
> >        case TypedObjectPrediction::Descr:
> > -        return descr().as<T>().type();
> 
> I don't think this is right. This code is also used for
> extractType<ScalarTypeDescr> and extractType<ReferenceTypeDescr>().
> 
> Just leave this as is, and your change to simdType() below is all we need.
> It was the only place instantiating extractType<SimdTypeDescr>() AFAICT.

Good catch! I mistakenly read SimdTypeDescr instead of TypeDescr when writing the code.

Other comments addressed as well.
3 files changed, 333 insertions(+), 462 deletions(-)

SIMD is still far away in the web assembly specification, which allows us to experiment with encodings. The one I am proposing in this patch isn't very compact but it makes things simpler. For a single operation SIMD.foo.bar, we now have:
- u8 Expr::SimdOp
- then the u8 ExprType (foo)
- then the u8 SimdOperation (bar)
- then the operands of the operation

It's not the best we can do but it is simple, as a start. We could also use a bitfield to compact the type/operation pair, maybe.

Also, there's a second opcode SimdConst because there's no such thing as "constant" or "literal" in the SimdOperation enumeration. We could use SimdOperation::Constructor there, but it's less optimal (WasmIonCompile would create a MSimdValueX4 out of that, which gets later folded into a MSimdConstant by GVN) and less compact (each slot is a new SIMD expression). Plus, it's symmetric with other wasm types, which is nice.
Attachment #8711002 - Flags: review?(luke)
Attachment #8711002 - Flags: review?(jolesen)
I think with wasm there should be a single opcode that is 1:1 with SIMD.foo.bar.  Then we could have a few static tables that can take us to and from this opcode and the component pieces of information.

You're right we should have a new *Const operator for SIMD constants, but I think for general wasm symmetry there should be one per SIMD vector type.
Keywords: leave-open
Comment on attachment 8711002 [details] [diff] [review]
Change SIMD opcodes encoding

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

FWIW, I think this is a great way of encoding the SIMD ops.

::: js/src/asmjs/AsmJS.cpp
@@ +2715,5 @@
> +        ExprType type;
> +        switch (simdType) {
> +          case SimdType::Int32x4:   type = ExprType::I32x4; break;
> +          case SimdType::Float32x4: type = ExprType::F32x4; break;
> +          case SimdType::Bool32x4:  type = ExprType::B32x4; break;

Looks like you need helper functions converting SimdType->ExprType, SimdType->ValType.
Attachment #8711002 - Flags: review?(jolesen) → review+
(In reply to Luke Wagner [:luke] from comment #8)
> I think with wasm there should be a single opcode that is 1:1 with
> SIMD.foo.bar.  Then we could have a few static tables that can take us to
> and from this opcode and the component pieces of information.

If you must, but just make sure to keep all those awful switches out of the compiler code.

We should definitely have something like Ben's writeSimdOp(Type, Op) in this patch, and corresponding decoder functions on the other end: isSimdOpcode() and decodeSimdOpcode() -> (SimdType, SimdOperation). Explicitly switching on 346 SIMD opcodes is error prone and tedious.

If those encode/decode functions can be written as math instead of big tables, the compiler will be smaller and faster for it.

We will always need a mapping from the wasm over-the-wire encoding to the compiler's internal encoding. The wire encoding should be dense, and for SIMD ops it will probably often be incomplete (Few programs will use 346 SIMD ops). For the compiler's internal encoding, a dense enumeration of all operations in not very practical. A sparse encoding that can easily be decoded into (Type, Op) pairs is much easier to work with.

Basically, a dense internal opcode space just forces you to do two table lookups: wire format -> internal opcode, and internal opcode -> useful information.

Compare to modern CPU design: Instructions are encoded densely to save on code size, and the first thing the CPU does is to decode instructions into micro-ops. The internal representation of micro-ops is typically much larger and sparse, but very regular. This simplifies the scheduling and execution logic that has to work with it.
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #11)
> We will always need a mapping from the wasm over-the-wire encoding to the
> compiler's internal encoding. The wire encoding should be dense, and for
> SIMD ops it will probably often be incomplete (Few programs will use 346
> SIMD ops). For the compiler's internal encoding, a dense enumeration of all
> operations in not very practical. A sparse encoding that can easily be
> decoded into (Type, Op) pairs is much easier to work with.

The current design is that the wasm binary format *is* the internal IR (that is handed from AsmJS.cpp to WasmIonCompile.cpp), so this is fixed: we need a dense index.  I think we should ultimately be able to just have 1 big switch though that pulls out the component information.

> Basically, a dense internal opcode space just forces you to do two table
> lookups: wire format -> internal opcode, and internal opcode -> useful
> information.

Since wire format == internal opcode, should just be one.
Comment on attachment 8711002 [details] [diff] [review]
Change SIMD opcodes encoding

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

Anyhow, this patch is probably a step in the right direction so I don't mean to hold it up, just say that I think we ultimately get to 1 opcode, no immediates.
Attachment #8711002 - Flags: review?(luke) → review+
Yes, it's indeed a step in the right direction! With the previous patch, we can just convert back and forth from (SimdType, SimdOperation) -> Expr.

When we want to add a new SIMD type in asm.js, we'll need to add one case to these 3 switches + a few places in AsmJS.cpp. How does this look, Jakob?

The only tradeoff is in WasmIonCompiler, where we have a switch over the Expr, then switch over the SimdOperation. This is a bit complicated, but I assume a C++ compiler with jump threading could realize that a given Expr conditions the SimdOperation and thus connects the two jumps into a single one. In any case, it hopefully is a temporary situation until we have the names-to-user-opcodes mapping.
Attachment #8712099 - Flags: review?(luke)
Attachment #8712099 - Flags: feedback?(jolesen)
Flags: needinfo?(bbouvier)
Another backout for this patch; msvc doesn't seem to support forward declarations of enum classes, or there's a namespace issue. In any case, the next patch removes the use of SimdOperation at all in WasmBinary.h, so we won't have to worry about it.
Comment on attachment 8712099 [details] [diff] [review]
4. Give each (Type, Operation) a single opcode

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

Nice!
Attachment #8712099 - Flags: review?(luke) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> Another backout for this patch; msvc doesn't seem to support forward
> declarations of enum classes, or there's a namespace issue. In any case, the
> next patch removes the use of SimdOperation at all in WasmBinary.h, so we
> won't have to worry about it.

We do have a few "enum class InlinableNative" forward decls already. Maybe some issue with overload resolution?
Comment on attachment 8712099 [details] [diff] [review]
4. Give each (Type, Operation) a single opcode

Looks good. At least this doesn't require too much typing to add more SIMD types.

The switches that simply return an enum in each case will be optimized into table lookups. You may be lucky enough that the inliner will recognize the jump threading opportunity in EmitSimdOp and inline all those calls into EmitExpr where the second switch level will be constant folded.
Attachment #8712099 - Flags: feedback?(jolesen) → feedback+
Depends on: 1243647
decoder is seeing crashes in SimdToExpr on one of the machines that runs GCC 4.7, but not on the other servers.

I wonder if this is the GCC bug that shows up when you use an enum with uint8 storage size. Unfortunately the patches here do use/add some of these enums. There's some discussion on this GCC bug in bug 1143966.

Benjamin, maybe you can work with decoder to figure out if this is indeed what's going on?
Flags: needinfo?(bbouvier)
Sure, yes. Let's move this discussion to another bug, I forgot to remove 'leave-open' on this one and all patches have landed (last two parts folded in a single patch).
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(bbouvier)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.