1112156 - TypePolicy: Unbox SIMD instruction operands.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

SIMD instructions might have teir inputs either coming from a SIMD
instruction, or from an object.  We should let the apply type policy decide
when to unbox these operands.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add new bailout kinds for MSimdUnbox.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Add MSimdUnbox to extract SIMD values from the TypedObjects.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Add SimdPolicy to extract SIMD operands based on the type of the instruction.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8545333 [details] [diff] [review]
Add MSimdUnbox to extract SIMD values from the TypedObjects.

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

::: js/src/jit/CodeGenerator.cpp
@@ +4404,5 @@
> +      case MIRType_Int32x4:
> +        masm.loadAlignedInt32x4(objectData, simd);
> +        break;
> +      case MIRType_Float32x4:
> +        masm.loadAlignedFloat32x4(objectData, simd);

I changed these for loadUnaligned, as SIMD TypedObjects are not properly aligned on x86.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8545332 [details] [diff] [review]
Add new bailout kinds for MSimdUnbox.

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

::: js/src/jit/Snapshots.cpp
@@ +20,5 @@
>  using namespace js::jit;
>  
>  // Snapshot header:
>  //
> +//   [vwu] bits ((n+1)-31]: recover instruction offset

While you're around, it might be worth making it explicit here that vwu stands for "variable-width unsigned" (the first explanation of this acronym is a lot of lines below)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8545333 [details] [diff] [review]
Add MSimdUnbox to extract SIMD values from the TypedObjects.

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

Unless i am missing something (in which case, forgive me and flag for review again), the bailout path is unused, and we never check the shape of the incoming object as being a TypedObject with the right TypeDescr. Is that correct?

::: js/src/jit/CodeGenerator.cpp
@@ +4398,5 @@
> +{
> +    Register object = ToRegister(lir->input());
> +    FloatRegister simd = ToFloatRegister(lir->output());
> +
> +    Address objectData(object, InlineTypedObject::offsetOfDataStart());

What ensures this is a valid read? The object could be any object...

::: js/src/jit/Lowering.cpp
@@ +3861,5 @@
> +    MOZ_ASSERT(ins->input()->type() == MIRType_Object);
> +    LUse in = useRegister(ins->input());
> +
> +    BailoutKind kind;
> +    MOZ_ASSERT(IsSimdType(ins->type()));

can you move this assert to the top of this function, please?

::: js/src/jit/MIR.h
@@ +2964,5 @@
> +    {
> +        return new(alloc) MSimdUnbox(op, type);
> +    }
> +
> +    AliasSet getAliasSet() const {

nit: MOZ_OVERRIDE

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8545334 [details] [diff] [review]
Add SimdPolicy to extract SIMD operands based on the type of the instruction.

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

r=me with the renaming

::: js/src/jit/TypePolicy.cpp
@@ +691,5 @@
> +bool
> +SimdPolicy<Op>::staticAdjustInputs(TempAllocator &alloc, MInstruction *ins)
> +{
> +    MIRType type = ins->type();
> +    MOZ_ASSERT(IsSimdType(type));

See below.

@@ +694,5 @@
> +    MIRType type = ins->type();
> +    MOZ_ASSERT(IsSimdType(type));
> +
> +    MDefinition *in = ins->getOperand(Op);
> +    if (in->type() == type)

Doesn't work for all operations: for instance, signMask expects a SIMD input and returns an int32, all comparison operators expect any SIMD type and return int32x4, etc.
So maybe just the name could be changed to something more specific than just "SimdPolicy": "SimdSameInputPolicy"? (we handled this pretty specifically by grouping operations of same type policy in asmjs, see also [0])

[0] https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/AsmJSValidate.cpp?from=AsmJSValidate.cpp#5318

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> Add new bailout kinds for MSimdUnbox.
> 
> ::: js/src/jit/Snapshots.cpp
> @@ +20,5 @@
> >  using namespace js::jit;
> >  
> >  // Snapshot header:
> >  //
> > +//   [vwu] bits ((n+1)-31]: recover instruction offset
> 
> While you're around, it might be worth making it explicit here that vwu
> stands for "variable-width unsigned" (the first explanation of this acronym
> is a lot of lines below)

This is the goal of a legend to stay away from the content while providing the definitions, right?
I do not think this add much value to the comment to explicit the first one.


(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 8545334 [details] [diff] [review]
> Add SimdPolicy to extract SIMD operands based on the type of the instruction.
> 
> @@ +694,5 @@
> > +    MIRType type = ins->type();
> > +    MOZ_ASSERT(IsSimdType(type));
> > +
> > +    MDefinition *in = ins->getOperand(Op);
> > +    if (in->type() == type)
> 
> Doesn't work for all operations: for instance, signMask expects a SIMD input
> and returns an int32, all comparison operators expect any SIMD type and
> return int32x4, etc.
> So maybe just the name could be changed to something more specific than just
> "SimdPolicy": "SimdSameInputPolicy"? (we handled this pretty specifically by
> grouping operations of same type policy in asmjs, see also [0])

SimdAutomorphismPolicy, SimdStableTypePolicy, SimdSameAsReturnedTypePolicy?

Another option is to keep SimdPolicy, but to expect that thisTypeSpecialization() is defined for each MSimd instruction which is using this specialization.

What do you think?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> > Add new bailout kinds for MSimdUnbox.
> > 
> > ::: js/src/jit/Snapshots.cpp
> > @@ +20,5 @@
> > >  using namespace js::jit;
> > >  
> > >  // Snapshot header:
> > >  //
> > > +//   [vwu] bits ((n+1)-31]: recover instruction offset
> > 
> > While you're around, it might be worth making it explicit here that vwu
> > stands for "variable-width unsigned" (the first explanation of this acronym
> > is a lot of lines below)
> 
> This is the goal of a legend to stay away from the content while providing
> the definitions, right?
> I do not think this add much value to the comment to explicit the first one.
> 
Hehe, this is a typical example of "I'm using an acronym I have defined, and then later on, i decide to reuse it before it's actually explicited".  See first uses at line 24 and lines 32 to 34 [0], and first definition [1] at line 97.  I am just asking to move the definition upwards, please.
 
[0] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Snapshots.cpp?from=Snapshots.cpp#24,32-34
[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Snapshots.cpp?from=Snapshots.cpp#97

> 
> (In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > Comment on attachment 8545334 [details] [diff] [review]
> > Add SimdPolicy to extract SIMD operands based on the type of the instruction.
> > 
> > @@ +694,5 @@
> > > +    MIRType type = ins->type();
> > > +    MOZ_ASSERT(IsSimdType(type));
> > > +
> > > +    MDefinition *in = ins->getOperand(Op);
> > > +    if (in->type() == type)
> > 
> > Doesn't work for all operations: for instance, signMask expects a SIMD input
> > and returns an int32, all comparison operators expect any SIMD type and
> > return int32x4, etc.
> > So maybe just the name could be changed to something more specific than just
> > "SimdPolicy": "SimdSameInputPolicy"? (we handled this pretty specifically by
> > grouping operations of same type policy in asmjs, see also [0])
> 
> SimdAutomorphismPolicy, SimdStableTypePolicy, SimdSameAsReturnedTypePolicy?
> 
> Another option is to keep SimdPolicy, but to expect that
> thisTypeSpecialization() is defined for each MSimd instruction which is
> using this specialization.
> 
> What do you think?

I would be all in for using thisTypeSpecialization(), as it would avoid proliferation of new SIMD type policies.  However, I am not sure this settles the issue: for instance, int32x4 binary operations expect 2 int32x4 and return one int32x4; while any float32x4 comparison operator expects two float32x4 vectors and return one int32x4 as well.  How would we differentiate these two sub-type policies?  A solution I can think of is to have a switch in the type policy function, but there's no need for the type specialization function in this case.  What do you think?
Otherwise, I'm good with SimdSameAsReturnedTypePolicy.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Add MSimdUnbox to extract SIMD values from the TypedObjects.

Right, I forgot to guard against bad inputs (I should probably add test for
it as well)

Delta:
 - Add MOZ_OVERRIDE on the getAliasSet function.
 - Add x64 ToPayload function to the MacroAssembler.
 - Inline the logic to check if the object is a SIMD object, and to verify
   if the type corresponds to the MIRType of MSimdUnbox.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8549734 [details] [diff] [review]
Add MSimdUnbox to extract SIMD values from the TypedObjects.

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

Did you forget to implement ToPayload on x86 (and make at least a stub on ARM)? Please add a test as well.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> Did you forget to implement ToPayload on x86 (and make at least a stub on
> ARM)? Please add a test as well.

No, they are already implemented on every platform except x64.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Add MSimdUnbox to extract SIMD values from the TypedObjects.

Delta:
 - Added a test case which bails because of 3 different reasons.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8550295 [details] [diff] [review]
Add MSimdUnbox to extract SIMD values from the TypedObjects.

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

Looks good, thanks! r=me with the test modified and the removal of masm-x64's ToType method, other remarks are less important.

::: js/src/jit-test/tests/ion/simd-unbox.js
@@ +26,5 @@
> +  if (i == 100)
> +    return f32x4(1, 1, 1, 1);
> +  // bailouts when the previous object is used as rhs, and lhs.
> +  return i32x4Add(lhs, rhs);
> +}

Can you add a test case for an inline transparent typed object which is *not* a SIMD object, please?

@@ +30,5 @@
> +}
> +
> +var arr_undef = [ i32x4(0, 1, 1, 2), i32x4(1, 1, 2, 3) ];
> +var arr_object = [ i32x4(0, 1, 1, 2), i32x4(1, 1, 2, 3) ];
> +var arr_badsimd = [ i32x4(0, 1, 1, 2), i32x4(1, 1, 2, 3) ];

Please ensure that each bailout occurs once and only once: var [undef_failed, object_failed, badsimd_failed] = [0,0,0];

@@ +33,5 @@
> +var arr_object = [ i32x4(0, 1, 1, 2), i32x4(1, 1, 2, 3) ];
> +var arr_badsimd = [ i32x4(0, 1, 1, 2), i32x4(1, 1, 2, 3) ];
> +for (var i = 0; i < 120; i++) {
> +  try {
> +    arr_undef[i + 2] = simdunbox_bail_undef(i, arr_undef[i], arr_undef[i + 1]);

fibonacci series <3

@@ +35,5 @@
> +for (var i = 0; i < 120; i++) {
> +  try {
> +    arr_undef[i + 2] = simdunbox_bail_undef(i, arr_undef[i], arr_undef[i + 1]);
> +  } catch (x) {
> +    arr_undef[i + 2] = i32x4(0, 0, 0, 0);

undef_failed += 1; // etc

@@ +49,5 @@
> +    arr_badsimd[i + 2] = simdunbox_bail_badsimd(i, arr_badsimd[i], arr_badsimd[i + 1]);
> +  } catch (x) {
> +    arr_badsimd[i + 2] = i32x4(0, 0, 0, 0);
> +  }
> +}

after the for loop, please assertEq(undef_failed, 1); // etc.

Actually, how about also asserting that the value at the last index of the SIMD array is the one that we expect? (i.e. check that arr_undef[119] (et al.) lanes are the ones we expect).

::: js/src/jit/CodeGenerator.cpp
@@ +4240,5 @@
> +    static_assert(!SimdTypeDescr::Opaque, "SIMD objects are transparent");
> +    masm.branchPtr(Assembler::NotEqual, clasp, ImmPtr(&InlineTransparentTypedObject::class_),
> +                   &bail);
> +
> +    // obj->type()->typeDescr()

Can you add in the comment that the addendumKind of the object is expected to be Addendum_TypeDescr for a typed object? Also, is there any easy way to assert this?

@@ +4257,5 @@
> +    }
> +#endif
> +    masm.branch32(Assembler::NotEqual, masm.ToPayload(typeDescrKind), Imm32(js::type::Simd), &bail);
> +
> +    // Convert the Simd MIRType to a SimdTypeDescr::Type.

nit: in comments, we usually wrote SIMD when referring to SIMD where it doesn't refer to variable / type names.  Not a big deal, but it'd be nice to stay consistent.

=> Convert the SIMD MIRType to a SimdTypeDescr::Type

@@ +4281,5 @@
> +        masm.branchTestInt32(Assembler::Equal, typeDescrType, &ok);
> +        masm.breakpoint();
> +        masm.bind(&ok);
> +    }
> +#endif

This debug check is nice and used several times in this function.  Please make it an helper function (static to this file?).

::: js/src/jit/LIR-Common.h
@@ +161,5 @@
> +{
> +  public:
> +    LIR_HEADER(SimdUnbox)
> +
> +    explicit LSimdUnbox(const LAllocation &obj, const LDefinition &temp)

no need for explicit, as there are 2 formal parameters

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +163,5 @@
>      /////////////////////////////////////////////////////////////////
> +    Address ToPayload(Address value) {
> +        return value;
> +    }
> +    Address ToType(Address value) {

unless i misread, ToType is unused. Can you please remove it? And maybe add a comment to ToPayload stating that this is here only for compatibility with 32bits platforms?

Comment 15

[Nicolas B. Pierron [:nbp]]

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fa33a987a6b7

(red on arm is fixed by making ToPayload public on ARM & MIPS)

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a50f81f29d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb33cbbe1d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4943a9ad69

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42a50f81f29d
https://hg.mozilla.org/mozilla-central/rev/fcb33cbbe1d2
https://hg.mozilla.org/mozilla-central/rev/8f4943a9ad69