1233111 - Add SIMD.Uint unsigned integer types to Javascript interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

The SIMD.js spec now contains unsigned integer types which should be added to the SpiderMonkey interpreter:

- SIMD.Uint8x16
- SIMD.Uint16x8
- SIMD.Uint32x4

These types have the same operations as the signed integer types.

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

This also includes adding new bitcasts to existing types: SIMD.*.fromUint*Bits(), and well as the unsigned-to-float conversion Float32x4.fromUint32x4().

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

Also add a shiftRightByScalar() to all signed and unsigned integer types as per the current SIMD.js spec.

It is probably best to keep the existing Int32x4.shiftRightLogicalByScalar() and Int32x4.shiftRightArithmeticByScalar() functions around for the benefit of asm.js, at least until asm.js gets the unsigned vector types too.

See also Bug 1201934.

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Add a new ToUint8() function. r?efaust

Review commit: https://reviewboard.mozilla.org/r/28297/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28297/

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Remove geometry altering SIMD conversions. r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28299/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28299/

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Share method lists for SIMD types. r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28301/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28301/

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Add unsigned SIMD types to interpreter. r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28303/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28303/

Comment 7

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Add ecma_7 shift tests. r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28305/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28305/

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Implement SIMD shiftRightByScalar(). r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28307/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28307/

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Attachment: MozReview Request: Bug 1233111 - Implement saturating arithmetic for SIMD. r?bbouvier

Review commit: https://reviewboard.mozilla.org/r/28309/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28309/

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8699604 [details]
MozReview Request: Bug 1233111 - Add a new ToUint8() function. r?efaust

https://reviewboard.mozilla.org/r/28297/#review25335

Looks good to me.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699605 [details]
MozReview Request: Bug 1233111 - Remove geometry altering SIMD conversions. r?bbouvier

https://reviewboard.mozilla.org/r/28299/#review25713

Good to see these gone.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699606 [details]
MozReview Request: Bug 1233111 - Share method lists for SIMD types. r?bbouvier

https://reviewboard.mozilla.org/r/28301/#review25715

Nice

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699607 [details]
MozReview Request: Bug 1233111 - Add unsigned SIMD types to interpreter. r?bbouvier

https://reviewboard.mozilla.org/r/28303/#review25717

Great! Very nice testing.

::: js/src/builtin/SIMD.h:279
(Diff revision 1)
> +  V(neg, (UnaryFunc<Uint8x16, Neg, Uint8x16>), 1)                                     \

uint8x16 doesn't have neg, according to the spec.

::: js/src/builtin/SIMD.h:299
(Diff revision 1)
> +  V(shiftRightLogicalByScalar, (BinaryScalar<Uint8x16, ShiftRightLogical>), 2)        \

There is only shiftRightByScalar, according to the spec, which has to be logical I guess for unsigned types?

::: js/src/builtin/SIMD.h:375
(Diff revision 1)
> +  V(neg, (UnaryFunc<Uint16x8, Neg, Uint16x8>), 1)                                     \

The same remarks as for uint8x16 also apply here.

::: js/src/builtin/SIMD.h:479
(Diff revision 1)
> +  V(neg, (UnaryFunc<Uint32x4, Neg, Uint32x4>), 1)                                     \

And here too.

::: js/src/builtin/TypedObject.js:722
(Diff revision 1)
> -          var s16 = callFunction(std_SIMD_Int8x16_extractLane, null, this, 7);
> +          var s16 = callFunction(std_SIMD_Int8x16_extractLane, null, this, 15);

Good catch!

::: js/src/tests/ecma_7/SIMD/binary-operations.js:373
(Diff revision 1)
> +    // Compute two 48-bit products. Truncate and combine them.

sweet

::: js/src/tests/ecma_7/SIMD/binary-operations.js:445
(Diff revision 1)
> +      print(v, w);

nit: rm print

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699608 [details]
MozReview Request: Bug 1233111 - Add ecma_7 shift tests. r?bbouvier

https://reviewboard.mozilla.org/r/28305/#review25727

Good catch. The changeset message tells me that you were aware that shiftRightArithmetic/Logical would be merge, so please ignore my review comment from the previous review in SIMD.h about merging these two for the unsigned types.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699609 [details]
MozReview Request: Bug 1233111 - Implement SIMD shiftRightByScalar(). r?bbouvier

https://reviewboard.mozilla.org/r/28307/#review25729

Approved.

::: js/src/asmjs/AsmJSValidate.cpp:4970
(Diff revision 1)
> +                IsSignedIntSimdType(opType) ? MSimdShift::rsh : MSimdShift::ursh, type);

The indent is wrong here:

case AsmJSSimdOperation_shiftRightByScalar:
  return CheckSimdBinary(f, call, opType,
                         IsSignedIntSimdType(opType) ? MSimdShift::rsh : MSimdShift::ursh,
                         type);
                         
and if even that doesn't fit in 100 chars,

case AsmJSSimdOperation_shiftRightByScalar:
  return CheckSimdBinary(f, call, opType,
                         IsSignedIntSimdType(opType)
                         ? MSimdShift::rsh
                         : MSimdShift::ursh,
                         type);

::: js/src/jit-test/tests/SIMD/shift.js:52
(Diff revision 1)
> +        assertEqX4(SIMD.Int32x4.shiftRightByScalar(v, 32), a.map(rsh(31)));

No tests for the other types? Not an issue, as I assume the plan is just to rename the operators when the Logical/Arithmethic versions go away.

::: js/src/jit/MCallOptimize.cpp:3107
(Diff revision 1)
> +        native == js::simd_int32x4_shiftRightByScalar)

style nit (either it should stand on one line if the line size can be < 100 chars, or have braces for the if body) but feel free to ignore, as it's temporary

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8699610 [details]
MozReview Request: Bug 1233111 - Implement saturating arithmetic for SIMD. r?bbouvier

https://reviewboard.mozilla.org/r/28309/#review25731

Thank you! Very nice set of patches overall (and this remotivated me to use MozReview, the UI isn't *too* bad anymore).

::: js/src/builtin/SIMD.cpp:704
(Diff revision 1)
> +template<typename T>

Do all our compilers support template method syntax? Last time I've tried, it wasn't, but it's been a while and might have changed.

Comment 17

[Jakob Stoklund Olesen [:jolesen]]

https://reviewboard.mozilla.org/r/28303/#review25717

> uint8x16 doesn't have neg, according to the spec.

Are you sure? According to http://tc39.github.io/ecmascript_simd/#simd-neg, neg is defined for all "integer types". This term includes both signed and unsigned types, c.f. http://tc39.github.io/ecmascript_simd/#simd-integer-type

Note that C/C++ is the same. For unsigned x, -x is defined and unsigned, using mod (UINT_MAX+1) arithmetic.

> There is only shiftRightByScalar, according to the spec, which has to be logical I guess for unsigned types?

Yes. I am trying to make a smoother transition for Emscripten by defining three right shift operators in this patch series:

- shiftRightLogicalByScalar
- shiftRightArithmeticByScalar
- shiftRightByScalar as per the spec

These three operators are defined for all integer types in this patch series.

Later when Emscripten has switched over to the standard shiftRightByScalar, I'll remove the non-standard ones with Bug 1201934 - Rename SIMD.js shiftRightLogicalByScalar to shiftRightByScalar.

Comment 18

[Jakob Stoklund Olesen [:jolesen]]

https://reviewboard.mozilla.org/r/28307/#review25729

> No tests for the other types? Not an issue, as I assume the plan is just to rename the operators when the Logical/Arithmethic versions go away.

The JIT currently only supports Int32x4. I intend to add tests here as Ion gets support for the new types.

Meanwhile, the functionality of these operations should be covered by tests/ecma_7/SIMD.

> style nit (either it should stand on one line if the line size can be < 100 chars, or have braces for the if body) but feel free to ignore, as it's temporary

Thanks, fixed.

Comment 19

[Jakob Stoklund Olesen [:jolesen]]

https://reviewboard.mozilla.org/r/28309/#review25731

> Do all our compilers support template method syntax? Last time I've tried, it wasn't, but it's been a while and might have changed.

This is a pretty straight-forward template function, not a method on a class. I don't think that will be an issue.

Comment 20

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699604 [details]
MozReview Request: Bug 1233111 - Add a new ToUint8() function. r?efaust

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28297/diff/1-2/

Comment 21

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699605 [details]
MozReview Request: Bug 1233111 - Remove geometry altering SIMD conversions. r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28299/diff/1-2/

Comment 22

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699606 [details]
MozReview Request: Bug 1233111 - Share method lists for SIMD types. r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28301/diff/1-2/

Comment 23

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699607 [details]
MozReview Request: Bug 1233111 - Add unsigned SIMD types to interpreter. r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28303/diff/1-2/

Comment 24

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699608 [details]
MozReview Request: Bug 1233111 - Add ecma_7 shift tests. r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28305/diff/1-2/

Comment 25

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699609 [details]
MozReview Request: Bug 1233111 - Implement SIMD shiftRightByScalar(). r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28307/diff/1-2/

Comment 26

[Jakob Stoklund Olesen [:jolesen]]

Comment on attachment 8699610 [details]
MozReview Request: Bug 1233111 - Implement saturating arithmetic for SIMD. r?bbouvier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28309/diff/1-2/

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #17)
> https://reviewboard.mozilla.org/r/28303/#review25717
> 
> > uint8x16 doesn't have neg, according to the spec.
> 
> Are you sure? According to http://tc39.github.io/ecmascript_simd/#simd-neg,
> neg is defined for all "integer types". This term includes both signed and
> unsigned types, c.f. http://tc39.github.io/ecmascript_simd/#simd-integer-type
> 
> Note that C/C++ is the same. For unsigned x, -x is defined and unsigned,
> using mod (UINT_MAX+1) arithmetic.

Ha, great, we've fine a discrepancy between the spec and the polyfill (I tend to use the polyfill as the reference implementation, but the spec has to be more precise). Opened https://github.com/tc39/ecmascript_simd/issues/310 for the polyfill implementation.

Comment 28

[Jakob Stoklund Olesen [:jolesen]]

https://reviewboard.mozilla.org/r/28303/#review25717

> Are you sure? According to http://tc39.github.io/ecmascript_simd/#simd-neg, neg is defined for all "integer types". This term includes both signed and unsigned types, c.f. http://tc39.github.io/ecmascript_simd/#simd-integer-type
> 
> Note that C/C++ is the same. For unsigned x, -x is defined and unsigned, using mod (UINT_MAX+1) arithmetic.

Oh, this is apparently a discrepancy between the spec and the polyfill. https://github.com/tc39/ecmascript_simd/issues/310

I will leave `neg` for unsigned types in for now. We can remove it if they change the spec.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9827c218b665
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8da4dede4a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/59ca29931b7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7de84f81fe57
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9259eb3f44a
https://hg.mozilla.org/integration/mozilla-inbound/rev/66bf206c4882
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb23aa681e99

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9827c218b665
https://hg.mozilla.org/mozilla-central/rev/c8da4dede4a6
https://hg.mozilla.org/mozilla-central/rev/59ca29931b7a
https://hg.mozilla.org/mozilla-central/rev/7de84f81fe57
https://hg.mozilla.org/mozilla-central/rev/b9259eb3f44a
https://hg.mozilla.org/mozilla-central/rev/66bf206c4882
https://hg.mozilla.org/mozilla-central/rev/bb23aa681e99

Comment 31

[Florian Scholz (Open Web Docs)]

Documentation updates:

Removed docs for conversion operators between vectors with different numbers of
lanes:
Int32x4.fromFloat64x2()
Float32x4.fromFloat64x2()
Float64x2.fromInt32x4()
Float64x2.fromFloat32x4()

Added docs for unsigned types and conversion operations:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8x16
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint16x8
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint32x4
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint32x4
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint32x4Bits
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint16x8Bits
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/fromUint8x16Bits

Added docs for the right shift function:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shiftRightByScalar

Marked old right shift functions as obsolete:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shiftRightArithmeticByScalar
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/shiftRightLogicalByScalar

Added docs for saturating arithmetic:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/addSaturate
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD/subSaturate