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.

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

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?

(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 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

(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 …

(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 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

(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.