1112154 - IonBuilder: Inline SIMD constructors.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

SIMD constructors are producing MIRType_Int32x4 or MIRType_Float32x4.  These
types cannot be captured in a resume point, as they are not expected by
baseline.  We should create an object in which these data should be stored.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Add MSimdBox and inline calls to SIMD constructors.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Add MSimdBox and inline calls to SIMD constructors.

Delta:
 - Remove isIonMode flag, based on Bug 1116491 fix.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8543298 [details] [diff] [review]
Add MSimdBox and inline calls to SIMD constructors.

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

Looks good to me, but just to be sure, I'd like jandem to give a quick look, to be sure I am not missing something obvious (and to learn, btw).
Do you plan to add SimdUnbox in this bug as well?

::: js/src/jit/CodeGenerator.cpp
@@ +4388,5 @@
> +      case MIRType_Float32x4:
> +        masm.storeAlignedFloat32x4(in, objectData);
> +        break;
> +      default:
> +        MOZ_CRASH("Unknown SIMD kind when generating init-code.");

nit: when generating box code? SimdBox code?

::: js/src/jit/LIR-Common.h
@@ +147,5 @@
> +        setOperand(0, simd);
> +        setTemp(0, temp);
> +    }
> +
> +    const LAllocation *input() {

nit: no need for this method, it's present in LInstructionHelper

::: js/src/jit/MCallOptimize.cpp
@@ +2633,5 @@
> +IonBuilder::InliningStatus
> +IonBuilder::inlineConstructSimdObject(CallInfo &callInfo, SimdTypeDescr *descr)
> +{
> +    // Generic constructor of SIMD valuesX4.
> +    if (callInfo.argc() == 4) {

How about returning early if callInfo.argc() != 4, instead?

@@ +2654,5 @@
> +        JSObject *templateObject = inspector->getTemplateObjectForClassHook(pc, descr->getClass());
> +        if (!templateObject)
> +            return InliningStatus_NotInlined;
> +
> +        // The previous assertion ensure this will never fail if we were able to

nit: ensures

@@ +2660,5 @@
> +        InlineTypedObject *inlineTypedObject = &templateObject->as<InlineTypedObject>();
> +        MOZ_ASSERT(&inlineTypedObject->typeDescr() == descr);
> +
> +        MDefinitionVector simdArgs(alloc());
> +        if (!simdArgs.reserve(callInfo.argc()))

we know the value of callInfo.argc(), please replace it here and below

@@ +2670,5 @@
> +            MDefinition *arg = callInfo.getArg(i);
> +            MInstruction *cast;
> +            if (elemType == MIRType_Int32)
> +                cast = MTruncateToInt32::New(alloc(), arg);
> +            else

in the else branch, please MOZ_ASSERT(elemType == MIRType_Float32);

@@ +2677,5 @@
> +            current->add(cast);
> +        }
> +
> +        // Create a MSimdValueX4, but prevent any foldsTo optimization which
> +        // might replace it by a non-supported instruction.

nit: outdated comment

::: js/src/jit/MIR.h
@@ +2896,5 @@
>  };
>  
> +// Generic way for constructing a SIMD object in IonMonkey, this instruction
> +// takes as argument an SIMD instruction and returns a new SIMD object which
> +// correspond to the MIRType of its operand.

nit: corresponds

@@ +2911,5 @@
> +      : MUnaryInstruction(op),
> +        templateObject_(templateObject),
> +        initialHeap_(initialHeap)
> +    {
> +        MOZ_ASSERT(IsSimdType(op->type()));

can you also assert that we're not simd-boxing a simd-box?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> ::: js/src/jit/MCallOptimize.cpp
> @@ +2633,5 @@
> > +IonBuilder::InliningStatus
> > +IonBuilder::inlineConstructSimdObject(CallInfo &callInfo, SimdTypeDescr *descr)
> > +{
> > +    // Generic constructor of SIMD valuesX4.
> > +    if (callInfo.argc() == 4) {
> 
> How about returning early if callInfo.argc() != 4, instead?

No because I think we still want to properly handle  callInfo.argc() == 1, with a fallible unbox.
And also because we might want to support callInfo.argc() == 2, for Float64x2.

> @@ +2660,5 @@
> > +        InlineTypedObject *inlineTypedObject = &templateObject->as<InlineTypedObject>();
> > +        MOZ_ASSERT(&inlineTypedObject->typeDescr() == descr);
> > +
> > +        MDefinitionVector simdArgs(alloc());
> > +        if (!simdArgs.reserve(callInfo.argc()))
> 
> we know the value of callInfo.argc(), please replace it here and below

Don't we want to keep this code generic, such that adding Float32x4 will work out of the box?

> @@ +2911,5 @@
> > +      : MUnaryInstruction(op),
> > +        templateObject_(templateObject),
> > +        initialHeap_(initialHeap)
> > +    {
> > +        MOZ_ASSERT(IsSimdType(op->type()));
> 
> can you also assert that we're not simd-boxing a simd-box?

This is already done by this assertion.  A MSimdBox should have a MIRType_Object type, and not a MIRType_Float32x4 or MIRType_Int32x4.  Otherwise, we would have problem with Phi nodes and with the resume point assertion added in Bug 1112153.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8543298 [details] [diff] [review]
Add MSimdBox and inline calls to SIMD constructors.

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

Looks good.

::: js/src/jit/CodeGenerator.cpp
@@ +4377,5 @@
> +    // we cannot use the same oolCallVM as visitNewTypedObject for allocating
> +    // SIMD Typed Objects if we are at the end of the nursery.
> +    Label bail;
> +    masm.createGCObject(object, temp, templateObject, initialHeap, &bail);
> +    bailoutFrom(&bail, lir->snapshot());

Maybe file a new bug to remove the bailout after bug 1112164 is fixed and mention it here? To make sure we don't forget about it when bug 1112164 is fixed.

::: js/src/jit/MCallOptimize.cpp
@@ +2642,5 @@
> +            break;
> +          case SimdTypeDescr::TYPE_FLOAT32:
> +            simdType = MIRType_Float32x4;
> +            break;
> +        }

Nit: I'd add a "default:" with MOZ_CRASH so that we will fail immediately when somebody adds a new type without adding it here.

@@ +2669,5 @@
> +        for (unsigned i = 0; i < callInfo.argc(); i++) {
> +            MDefinition *arg = callInfo.getArg(i);
> +            MInstruction *cast;
> +            if (elemType == MIRType_Int32)
> +                cast = MTruncateToInt32::New(alloc(), arg);

Shouldn't we do this in the MSimdValueX4 type policy?

::: js/src/jit/MIR.h
@@ +2895,5 @@
>      }
>  };
>  
> +// Generic way for constructing a SIMD object in IonMonkey, this instruction
> +// takes as argument an SIMD instruction and returns a new SIMD object which

Nit: s/an/a

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> ::: js/src/jit/MCallOptimize.cpp
> @@ +2642,5 @@
> > +            break;
> > +          case SimdTypeDescr::TYPE_FLOAT32:
> > +            simdType = MIRType_Float32x4;
> > +            break;
> > +        }
> 
> Nit: I'd add a "default:" with MOZ_CRASH so that we will fail immediately
> when somebody adds a new type without adding it here.

Wouldn't this be better to keep a compiler error due to the incomplete switch case?

> @@ +2669,5 @@
> > +        for (unsigned i = 0; i < callInfo.argc(); i++) {
> > +            MDefinition *arg = callInfo.getArg(i);
> > +            MInstruction *cast;
> > +            if (elemType == MIRType_Int32)
> > +                cast = MTruncateToInt32::New(alloc(), arg);
> 
> Shouldn't we do this in the MSimdValueX4 type policy?

Indeed, I thought about it and I forgot …

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> (In reply to Jan de Mooij [:jandem] from comment #5)
> > ::: js/src/jit/MCallOptimize.cpp
> > @@ +2642,5 @@
> > > +            break;
> > > +          case SimdTypeDescr::TYPE_FLOAT32:
> > > +            simdType = MIRType_Float32x4;
> > > +            break;
> > > +        }
> > 
> > Nit: I'd add a "default:" with MOZ_CRASH so that we will fail immediately
> > when somebody adds a new type without adding it here.
> 
> Wouldn't this be better to keep a compiler error due to the incomplete
> switch case?

Agreed, and this will be triggered with warning-as-errors on at least gcc, i think.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8543298 [details] [diff] [review]
Add MSimdBox and inline calls to SIMD constructors.

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

::: js/src/jit/CodeGenerator.cpp
@@ +4382,5 @@
> +
> +    Address objectData(object, InlineTypedObject::offsetOfDataStart());
> +    switch (type) {
> +      case MIRType_Int32x4:
> +        masm.storeAlignedInt32x4(in, objectData);

Apparently this storeAligned is wrong on x86.  Even if our objects are 16 bytes aligned, and that the JSObjects are also 16 bytes on x64, this is not the case on x86, where our JSObjects are 8 bytes headers.

The data_ fields of the InlineTypedObject is thus shifted from being aligned to be unaligned.  So I will just change this code to use unaligned store instead, in order to work-around this issue for the moment.

At the end, we should only be doing these operations infrequently, so hopefully, only the function boundaries and the unknown types would be the pain points here.

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

Comment 9

[Nicolas B. Pierron [:nbp]]

(Try) https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44d3548b55b2
(inbound) https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3b15d06665

 - ARM is fixed by not collecting the template object in baseline, which prevent the inlining of the SIMD constructor.
 - x86 is fixed by doing a storeUnaligned for writting into the InlineTypedObject data field.
 - SimdScalarPolicy is added, the code moved from the MCallOptimize.cpp to TypePolicy.cpp.  I had to move the assertions made in the constructor of MSimdValueX4 to MSimdValueX4::NewAsmJS, as discuss with benjamin.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac3b15d06665