Some SIMD operations don't need a lot of work to be used in Ion. These operations are the arithmetic operations, comparisons, et al. Examples of operations which won't be as easy to implement in Ion: load/store, shifts (our shifts MIR nodes expect constant shift count, at the moment), and a few others. I'll take care of the easy ones in this bug.

Not quite sure about this one, as i kinda remember people didn't want to introduce templating in this place. However, the two inlineSimdBinary* functions are a copy/pasto, at this time... (feel free to transform in a review if you're fine with it -- otherwise we'd need something else)

I am not entirely sure about the usefulness of this patch, considering his tradeoffs... Advantages: - introduces a nice framework for testing SIMD operations in jit/ - tests correctness of operations Drawbacks: - slows down testing (--tbpl takes ~5 secondes each, on my machine) - we can't check operations actually are inlined, by watching iongraph spew, as it is flooded by other harness functions. Any opinions?

Comment on attachment 8566575 [details] [diff] [review] 3.templatize.patch Review of attachment 8566575 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.h @@ +806,5 @@ > // SIMD intrinsics and natives. > InliningStatus inlineConstructSimdObject(CallInfo &callInfo, SimdTypeDescr *target); > + > + template<typename MIRNode> > + InliningStatus inlineBinarySimd(CallInfo &callInfo, JSNative native, style-nit: template␣<typename … ::: js/src/jit/MCallOptimize.cpp @@ +258,5 @@ > > // Simd functions > +#define INLINE_INT32X4_SIMD_ARITH_(OP) \ > + if (native == js::simd_int32x4_##OP) \ > + return inlineBinarySimd<MSimdBinaryArith>(callInfo, native, MSimdBinaryArith::Op_##OP, \ Is the template specialization really needed? I think the rule only applies if the typename is an argument, but this might be interesting to test ;) @@ +2902,5 @@ > // type, or a Value, then the applyTypes phase will add a fallible box & > // unbox sequence. This does not matter much as the binary bitwise > // instruction is supposed to produce a TypeError once it is called. > + MIRNode *ins = MIRNode::New(alloc(), callInfo.getArg(0), callInfo.getArg(1), op, > + SimdTypeDescrToMIRType(type)); MIRNode is confusing, especially since this is an instruction, use MTemplateParam maybe.

Comment on attachment 8566572 [details] [diff] [review] 1.bitwise.opname.patch Review of attachment 8566572 [details] [diff] [review]: ----------------------------------------------------------------- Hooray for extraNames!

Comment on attachment 8566574 [details] [diff] [review] 2.inline.float32x4.bitwise.specific-arith.patch Review of attachment 8566574 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Especially with the 2 other patches.

(I didn't worry about using macros for all the |if (...) ... |, because all of this will likely get folded in the latest patch of this bug)

Oops, tests were actually not passing, because there were a few mistakes in it...

FloatingPointPolicy is just a plain copy of TypeSpecializationData, so let's just replace it.

Comment on attachment 8567181 [details] [diff] [review] 8.inline.conversions.patch Actually, this one test doesn't pass with --ion-eager, and looks very timing related. Is there anything real bad you can spot in this patch?

Comment on attachment 8567118 [details] [diff] [review] 6.inline.unary.patch Review of attachment 8567118 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineIC.cpp @@ +9168,5 @@ > ARITH_FLOAT32X4_SIMD_OP(ADD_FLOAT32X4_SIMD_OP_NAME_) > + BITWISE_COMMONX4_SIMD_OP(ADD_FLOAT32X4_SIMD_OP_NAME_) > + || native == js::simd_float32x4_abs || native == js::simd_float32x4_sqrt > + || native == js::simd_float32x4_reciprocal || native == js::simd_float32x4_reciprocalSqrt > + || native == js::simd_float32x4_not || native == js::simd_float32x4_neg) Any reason to not use a Macro?

Comment on attachment 8567171 [details] [diff] [review] 6.2.unary.tests.patch Review of attachment 8567171 [details] [diff] [review]: ----------------------------------------------------------------- Merge it with the part 6.

Comment on attachment 8567119 [details] [diff] [review] 7.spew.simd.patch Review of attachment 8567119 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8567181 [details] [diff] [review] 8.inline.conversions.patch Review of attachment 8567181 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Add tests and r=me.

I'm hitting a case in the tests for conversions, where we emit a check in assembly that pushes all volatile registers (in emitObjectOrStringResultChecks), but only 64 bits of xmm registers are spilled onto (resp. reloaded from) the stack, and then one of the register values gets corrupted. Marking 1112164 as a blocker, as we need to make sure the engine knows it needs to spill high lanes of xmm registers by default. (just confirmed it's fixed by the folded patch in bug 1112164)

4 first patches: Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87012f4d14a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0408a35f9cd7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9436a637f882 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a69728808f9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3b50e9d9e2

https://hg.mozilla.org/mozilla-central/rev/0408a35f9cd7 https://hg.mozilla.org/mozilla-central/rev/9436a637f882 https://hg.mozilla.org/mozilla-central/rev/5a69728808f9 https://hg.mozilla.org/mozilla-central/rev/fa3b50e9d9e2

A few type checks that would have shown useful, to tell me i was missing some TypePolicy...

This inlines .x, .y, .z, .w and .signMask get property accesses. Unfortunately, it reveals a few bugs in LSRA and I have spent a few hours to investigate yesterday but I couldn't even get a grasp of what the issue is (tests pass fine with backtracking allocator, making it a requirement for this patch). It seems that at some point, all registers are busy, and regalloc chooses a blocked register which is used sufficiently far away, but it doesn't spill/reload it between the two uses... Shu, is my (tiny) use of the tracking infra correct here? (in IonBuilder.cpp)

As stated in comment 27, blocked by backtracking allocator landing, as the last patches revealed a few issues in LSRA...

Comment on attachment 8568433 [details] [diff] [review] 10.comparisons.patch Review of attachment 8568433 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MCallOptimize.cpp @@ +286,5 @@ > + return inlineCompSimd(callInfo, native, MSimdBinaryComp::OP, SimdTypeDescr::TYPE_INT32); \ > + if (native == js::simd_float32x4_##OP) \ > + return inlineCompSimd(callInfo, native, MSimdBinaryComp::OP, SimdTypeDescr::TYPE_FLOAT32); > + COMP_COMMONX4_SIMD_OP(INLINE_SIMD_COMPARISON_) > +#undef INLINE_SIMD_COMPARISON_ nit: For multiline macros, add a new line between the macro and the use of the macro. Do the same above.

Comment on attachment 8568435 [details] [diff] [review] 12.setters.patch Review of attachment 8568435 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MCallOptimize.cpp @@ +293,5 @@ > + if (native == js::simd_int32x4_with##LANE) \ > + return inlineSimdWith(callInfo, native, SimdLane::Lane##LANE, SimdTypeDescr::TYPE_INT32); \ > + if (native == js::simd_float32x4_with##LANE) \ > + return inlineSimdWith(callInfo, native, SimdLane::Lane##LANE, SimdTypeDescr::TYPE_FLOAT32); > + INLINE_SIMD_SETTER_(X) nit: Same as previous patch. Add a new line above.

Annnnnnnd that's the last one! I've fixed the new-lines-after-macro nits in this patch, to avoid the need of rebasing over 6 patches :)

Comment on attachment 8568439 [details] [diff] [review] 14.getters.patch Review of attachment 8568439 [details] [diff] [review]: ----------------------------------------------------------------- Use of tracking stuff is good; feel free to add more specific SIMD-specific outcomes! ::: js/src/jit/IonBuilder.cpp @@ +9964,5 @@ > +{ > + MOZ_ASSERT(!*emitted); > + > + TypedObjectPrediction objPrediction = typedObjectPrediction(obj); > + if (objPrediction.isUseless() || objPrediction.kind() != type::Simd) If you'd like to report more specific reasons, you could crib the pattern in IonBuilder::typedObjectHasField here. objPrediction.isUseless() would be tracked as AccessNotTypedObject, and objPrediction.kind() != type::Simd would be tracked as NotSimd (a new outcome). @@ +9968,5 @@ > + if (objPrediction.isUseless() || objPrediction.kind() != type::Simd) > + return true; > + > + MIRType type = SimdTypeDescrToMIRType(objPrediction.simdType()); > + if (type == MIRType_Undefined) Similarly, this could be tracked as NotSimd. @@ +9979,5 @@ > + MSimdSignMask *ins = MSimdSignMask::New(alloc(), obj, type); > + current->add(ins); > + current->push(ins); > + trackOptimizationSuccess(); > + return *emitted = true; I would prefer *emitter = true; return true; over returning an assignment expression. @@ +9993,5 @@ > + lane = LaneZ; > + } else if (name == names.w) { > + lane = LaneW; > + } else { > + // Unknown getprop access on a SIMD value This would warrant a new outcome as well, something like UnknownSimdLane.

Comment on attachment 8568439 [details] [diff] [review] 14.getters.patch Review of attachment 8568439 [details] [diff] [review]: ----------------------------------------------------------------- Address shu's feedback and r=me.

Comment on attachment 8568684 [details] [diff] [review] 15.select.bitselect.patch Review of attachment 8568684 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MCallOptimize.cpp @@ +333,5 @@ > return inlineSimdSplat(callInfo, native, SimdTypeDescr::TYPE_INT32); > if (native == js::simd_float32x4_splat) > return inlineSimdSplat(callInfo, native, SimdTypeDescr::TYPE_FLOAT32); > > + typedef bool IsElementWise; fun and clever!

Comment on attachment 8568685 [details] [diff] [review] 16.moar.macros.patch Review of attachment 8568685 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.h @@ +207,5 @@ > _(reciprocal) \ > + _(reciprocalSqrt) > +#define FOREACH_FLOAT32X4_SIMD_OP(_) \ > + BINARY_ARITH_FLOAT32X4_SIMD_OP(_)\ > + UNARY_ARITH_FLOAT32X4_SIMD_OP(_) \ nit: Unless the order matters (which I hope not), move the UNARY before the BINARY. Same thing for the macro definitions above. @@ +231,5 @@ > _(withY) \ > _(withZ) \ > _(withW) > +// TODO: remove when all SIMD calls are inlined (bug 1112155) > +#define ION_COMMONX4_SIMD_OP(_) \ We could be greedy in baseline and register all SIMD operators, even if we do not yet inline the functions in Ion. Also, I think the only reason why you had to do the separation with COMP_COMMONX4_SIMD_OP(_) is mostly because the Macro of SIMD.h are referring to the SIMD Type, where the macro for baseline are referring to the returned type. I think these macro would be better named if the return type was mentioned. #define COMMONX4_TO_INT32X4_SIMD_OP(_) \ COMP_COMMONX4_SIMD_OP(_) #define INT32X4_TO_FLOAT32X4_SIMD_OP(_) \ _(fromFloat32x4) \ _(fromFloat32x4Bits) #define FLOAT32X4_TO_INT32X4_SIMD_OP(_) \ _(fromInt32x4) \ _(fromInt32x4Bits) #define COMMONX4_TO_SAMEX4_SIMD_OP(_)\ ARITH_COMMONX4_SIMD_OP(_) \ BITWISE_COMMONX4_SIMD_OP(_) \ WITH_COMMONX4_SIMD_OP(_) \ _(bitselect) \ _(select) \ _(splat) \ _(not) \ _(neg) what do you think?

(In reply to Nicolas B. Pierron [:nbp] from comment #36) > Comment on attachment 8568685 [details] [diff] [review] > 16.moar.macros.patch > > Review of attachment 8568685 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/builtin/SIMD.h > @@ +207,5 @@ > > _(reciprocal) \ > > + _(reciprocalSqrt) > > +#define FOREACH_FLOAT32X4_SIMD_OP(_) \ > > + BINARY_ARITH_FLOAT32X4_SIMD_OP(_)\ > > + UNARY_ARITH_FLOAT32X4_SIMD_OP(_) \ > > nit: Unless the order matters (which I hope not), move the UNARY before the > BINARY. Same thing for the macro definitions above. Done. > > @@ +231,5 @@ > > _(withY) \ > > _(withZ) \ > > _(withW) > > +// TODO: remove when all SIMD calls are inlined (bug 1112155) > > +#define ION_COMMONX4_SIMD_OP(_) \ > > We could be greedy in baseline and register all SIMD operators, even if we > do not yet inline the functions in Ion. We could, but currently, looking at the ops which are in the FOREACH_COMMONX4 section acts as a TODO list of remaining operations to inline. If you really want it, I could move these functions, but I do prefer doing them one at a time. > > Also, I think the only reason why you had to do the separation with > COMP_COMMONX4_SIMD_OP(_) is mostly because the Macro of SIMD.h are referring > to the SIMD Type, where the macro for baseline are referring to the returned > type. > > I think these macro would be better named if the return type was mentioned. > > #define COMMONX4_TO_INT32X4_SIMD_OP(_) \ > COMP_COMMONX4_SIMD_OP(_) > > #define INT32X4_TO_FLOAT32X4_SIMD_OP(_) \ > _(fromFloat32x4) \ > _(fromFloat32x4Bits) > > #define FLOAT32X4_TO_INT32X4_SIMD_OP(_) \ > _(fromInt32x4) \ > _(fromInt32x4Bits) > > #define COMMONX4_TO_SAMEX4_SIMD_OP(_)\ > ARITH_COMMONX4_SIMD_OP(_) \ > BITWISE_COMMONX4_SIMD_OP(_) \ > WITH_COMMONX4_SIMD_OP(_) \ > _(bitselect) \ > _(select) \ > _(splat) \ > _(not) \ > _(neg) > > what do you think? I think it makes sense for the outlier, i.e. comparisons. I've let it as is for the other macros, as they all obey the obvious rule. I don't have a strong opinion here, except that renaming macros is a pain, and having long macro names is a pain as well, but you could convince me that's really important, if it were the case :) In other patches: I've addressed Shu's feedback (thanks!) and added a JitSupportSimd check in getPropTrySimdGetter as well (otherwise it would fail on ARM -- note that operators will naturally fail on ARM, as we won't have the template object when trying to inline). I have also noticed I haven't added checks in tests, to make sure SIMD isn't undefined (as it is on non-nightly builds). Will do in the upcoming patch.

Comment on attachment 8569796 [details] [diff] [review] 16.macros.patch Review of attachment 8569796 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/SIMD/check.js @@ +1,2 @@ > +if (!this.hasOwnProperty("SIMD")) > + quit(); oops, this file ain't belong here. I removed it locally and moved it to the check patch in the other bug...

Try build with All The Things (tm): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a4db0d64095

Comment on attachment 8569796 [details] [diff] [review] 16.macros.patch Review of attachment 8569796 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/SIMD/with.js @@ +1,2 @@ > +if (!this.hasOwnProperty("SIMD")) > + quit(); Would it make sense to move these lines into lib/simd.js ? ::: js/src/jit/MCallOptimize.cpp @@ +261,5 @@ > if (native == js::simd_float32x4_##OP) \ > return inlineBinarySimd<MSimdBinaryArith>(callInfo, native, MSimdBinaryArith::Op_##OP, \ > SimdTypeDescr::TYPE_FLOAT32); > + > + BINARY_ARITH_FLOAT32X4_SIMD_OP(INLINE_FLOAT32X4_SIMD_ARITH_) nit: I would prefer if you can move this statement before the list of #undef. @@ +267,5 @@ > +#define INLINE_SIMD_ARITH_(OP) \ > + INLINE_FLOAT32X4_SIMD_ARITH_(OP) \ > + if (native == js::simd_int32x4_##OP) \ > + return inlineBinarySimd<MSimdBinaryArith>(callInfo, native, MSimdBinaryArith::Op_##OP, \ > + SimdTypeDescr::TYPE_INT32); nit: I think this code is easier to read when we have macros of code, and maybe macros of macros, but not a mix of macro of code+macro. Add INLINE_INT32X4_SIMD_ARITH_(OP) back, and use it here.

(In reply to Nicolas B. Pierron [:nbp] from comment #42) > Comment on attachment 8569796 [details] [diff] [review] > 16.macros.patch > > Review of attachment 8569796 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit-test/tests/SIMD/with.js > @@ +1,2 @@ > > +if (!this.hasOwnProperty("SIMD")) > > + quit(); > > Would it make sense to move these lines into lib/simd.js ? Indeed better, good idea! Fixed all other nits as well. Will wait a bit for all the SIMD patches to stick, and then land this one (try above is green).

https://hg.mozilla.org/mozilla-central/rev/d15386307e40 https://hg.mozilla.org/mozilla-central/rev/99b8284d006e https://hg.mozilla.org/mozilla-central/rev/f9a0bce65a8d https://hg.mozilla.org/mozilla-central/rev/6ab15d0a97c2 https://hg.mozilla.org/mozilla-central/rev/b0a80dbf11ca https://hg.mozilla.org/mozilla-central/rev/771f49cc99e7 https://hg.mozilla.org/mozilla-central/rev/4a1497c0f16e https://hg.mozilla.org/mozilla-central/rev/3100e1df4f0f https://hg.mozilla.org/mozilla-central/rev/26fff1612f87 https://hg.mozilla.org/mozilla-central/rev/0036dc1b7bdf https://hg.mozilla.org/mozilla-central/rev/8db64625dd3a https://hg.mozilla.org/mozilla-central/rev/97ce84348766