Calls to SIMD.float32x4.add are inlined specially if: 1) Both operands are typed objects of type T where T = either float32x4 or int32x4 2) In that case, insert MLoadVector for each argument (using existing code that skips indirection as a model – detecting MDerivedTypeObject and MVectorTypedObject and shortcircuit) 3) Insert MAddVector with unboxed operands 4) Insert MVectorTypedObject to box result

Rebased with inbound.

Rebased with inbound.

Niko, thanks a lot for the review. I rebased the two patches you have reviewed with inbound, could you take another look?

Comment on attachment 8355457 [details] [diff] [review] 0003-Introduce-ToX4-ToX4TypedObject-MIR-and-SIMDInputsPol.patch Review of attachment 8355457 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +4148,4 @@ > } > }; > > +class MToX4 Please add a comment explaining what this MIR does. The name alone isn't immediately clear. @@ +4151,5 @@ > +class MToX4 > + : public MUnaryInstruction > +{ > + protected: > + MToX4(MDefinition *def, MIRType type) Please add an assertion regarding the type of `def`. It should always be an object, right? @@ +4161,5 @@ > + > + public: > + INSTRUCTION_HEADER(ToX4) > + static MToX4 *New(TempAllocator &alloc, MDefinition *def, MIRType type) > + { Nit: put opening brace on previous line

Comment on attachment 8381425 [details] [diff] [review] 0002-Set-up-SIMD-inlining-infrastructure.patch Review of attachment 8381425 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonTypes.h @@ +167,5 @@ > case MIRType_Magic: > return JSVAL_TYPE_MAGIC; > + case MIRType_Float32x4: > + case MIRType_Int32x4: > + return JSVAL_TYPE_OBJECT; It is awkward to have MIR types with no direct JSVAL_TYPE equivalent. I wonder if we can somehow factor this better to prevent this scenario from arising. A quick grep through the source reveals that ValueTypeFromMIRType() is used in a lot of places so it's not readily evident to me, but I think it'd be worth the trouble. This setup worries me. *Probably* a different bug though. ::: js/src/jit/MIR.h @@ +3997,5 @@ > + } > +}; > + > +#define MSIMD_UNARY_FUNCTION_LIST(V) \ > + V(Float32x4Abs, "float32x4.abs", MIRType_Float32x4, MIRType_Float32x4) \ Can we somehow derive this information the function lists in SIMD.h? I'd prefer not to repeat it. It feels like we should be able to have one master list with the function and its expected argument types in some form that can be converted as needed. However, looking at SIMD.h, it's not obvious at a quick glance how to avoid the repeated data, so maybe this is just an idle wish.

(In reply to Niko Matsakis [:nmatsakis] from comment #8) > Comment on attachment 8381425 [details] [diff] [review] > 0002-Set-up-SIMD-inlining-infrastructure.patch > > Review of attachment 8381425 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/IonTypes.h > @@ +167,5 @@ > > case MIRType_Magic: > > return JSVAL_TYPE_MAGIC; > > + case MIRType_Float32x4: > > + case MIRType_Int32x4: > > + return JSVAL_TYPE_OBJECT; > > It is awkward to have MIR types with no direct JSVAL_TYPE equivalent. I > wonder if we can somehow factor this better to prevent this scenario from > arising. A quick grep through the source reveals that ValueTypeFromMIRType() > is used in a lot of places so it's not readily evident to me, but I think > it'd be worth the trouble. This setup worries me. *Probably* a different bug > though. > ::: js/src/jit/MIR.h > @@ +3997,5 @@ > > + } > > +}; > > + > > +#define MSIMD_UNARY_FUNCTION_LIST(V) \ > > + V(Float32x4Abs, "float32x4.abs", MIRType_Float32x4, MIRType_Float32x4) \ > > Can we somehow derive this information the function lists in SIMD.h? I'd > prefer not to repeat it. It feels like we should be able to have one master > list with the function and its expected argument types in some form that can > be converted as needed. However, looking at SIMD.h, it's not obvious at a > quick glance how to avoid the repeated data, so maybe this is just an idle > wish. I have tried to record all the information in SIMD.h, but this will get the macro item hard to read. Instead currently we could rely on the compiler to detect the missing function id in MIR Id list as when we add a new item in SIMD.h, the corresponding MIR id should be defined, otherwise compiler will complain at MCallOptimize.cpp.

Try run for first 2 parts: https://tbpl.mozilla.org/?tree=Try&rev=58f7c1fee973

Haitao -- it looks like there are some build failures in that try run: https://tbpl.mozilla.org/php/getParsedLog.php?id=35427108&tree=Try

Fixed the style checking failure.

Niko, thanks for trying this. I have fixed the style checking failure and will check the style before submitting patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3194cb1d73f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/7efaabf97f0c

https://hg.mozilla.org/mozilla-central/rev/3194cb1d73f6 https://hg.mozilla.org/mozilla-central/rev/7efaabf97f0c

Comment on attachment 8381408 [details] [diff] [review] 0001-Use-macros-to-declare-and-define-SIMD-functions.patch AFAICT, these were checked in.

Comment on attachment 8384372 [details] [diff] [review] 0002-Set-up-SIMD-inlining-infrastructure.patch Review of attachment 8384372 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +4023,5 @@ > + V(Int32x4GetSignMask, "int32x4.getSignMask", MIRType_Int32, MIRType_Int32x4) \ > + V(Int32x4GetFlagX, "int32x4.getFlagX", MIRType_Boolean, MIRType_Int32x4) \ > + V(Int32x4GetFlagY, "int32x4.getFlagY", MIRType_Boolean, MIRType_Int32x4) \ > + V(Int32x4GetFlagZ, "int32x4.getFlagZ", MIRType_Boolean, MIRType_Int32x4) \ > + V(Int32x4GetFlagW, "int32x4.getFlagW", MIRType_Boolean, MIRType_Int32x4) I thought I already mentioned that this was a bad idea to declare so many MIR node when we have so little differences between them? I clearly remember suggesting that you should use a MVector* over IRC. Why did this changes landed?

All these patches should be backed out because they are not-implementing the Lowering & CodeGen parts. And as mentioned also to haitao, having generic SIMD instructions that we dispatch based on their number of arguments is not an option! This is like dispatching on MUnary & MBinary.

Please include all the test cases produced by decoder's fuzzer, see all the depend bugs. Then split your patch to implement each function at a time, from the MIR to the CodeGen. And with each patch, add the corresponding test case which ensure that the newly inlined function is exercised by a test case. In the mean time, To let decoder's fuzzer be useful for the rest of the engine, I backed out the second part of this bug. https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb9cb300800

Comment on attachment 8384372 [details] [diff] [review] 0002-Set-up-SIMD-inlining-infrastructure.patch Reset checking flag to reflect the backed out patch.

nbp -- I discussed with jandem and we disagreed that multiple mir would be superior. As for checking in with unimplemented code, I do apologize, that's my fault. I should not have given r+, I missed that aspect of the patch. Thanks for backing it out.

nbp and niko, sorry it is my fault to submit the checkin request for the second patch "Set-up-SIMD-inlining-infrastructure.patch" which broke the decoder's fuzzer. I will continue to work on the SIMD stuff from next week and address all your comments and concerns before submitting checkin request.

This is now being done in bug 1112155, which is a meta bug for all the different operations.