1134638 - SIMD: Inline "simple" SIMD operations in Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.bitwise.opname.patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.inline.float32x4.bitwise.specific-arith.patch

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.templatize.patch

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)

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.tests.patch

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 5

[Nicolas B. Pierron [:nbp]]

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 6

[Dan Gohman [:sunfish]]

Comment on attachment 8566572 [details] [diff] [review]
1.bitwise.opname.patch

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

Hooray for extraNames!

Comment 7

[Nicolas B. Pierron [:nbp]]

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.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5.opname.unary.patch

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6.inline.unary.patch

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

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 7.spew.simd.patch

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: inline.unary.tests.patch

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6.2.unary.tests.patch

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

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 8.inline.conversions.patch

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 9.driveby.cleanup.patch

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

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

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 16

[Nicolas B. Pierron [:nbp]]

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 17

[Nicolas B. Pierron [:nbp]]

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 18

[Dan Gohman [:sunfish]]

Comment on attachment 8567119 [details] [diff] [review]
7.spew.simd.patch

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

Looks good.

Comment 19

[Nicolas B. Pierron [:nbp]]

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.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

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)

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 10.comparisons.patch

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 11.typechecks.patch

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

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 12.setters.patch

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 13.splat.patch

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 14.getters.patch

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)

Comment 28

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 29

[Nicolas B. Pierron [:nbp]]

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 30

[Nicolas B. Pierron [:nbp]]

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.

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 15.select.bitselect.patch

Comment 32

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 16.moar.macros.patch

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 33

[Shu-yu Guo [:shu]]

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 34

[Nicolas B. Pierron [:nbp]]

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 35

[Nicolas B. Pierron [:nbp]]

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 36

[Nicolas B. Pierron [:nbp]]

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?

Comment 38

[Benjamin Bouvier [:bbouvier] (inactive)]

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

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 16.macros.patch

Comment 40

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 41

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 42

[Nicolas B. Pierron [:nbp]]

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.

Comment 43

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 44

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=97ce84348766

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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