Add SIMD.Uint unsigned integer types to Javascript interpreter

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jolesen, Assigned: jolesen)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla46
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [DocArea=JS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → jolesen
Blocks: 894105
(Assignee)

Comment 1

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

Updated

2 years ago
Blocks: 1201934
(Assignee)

Comment 2

2 years ago
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.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Comment 3

2 years ago
Created attachment 8699604 [details]
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/
Attachment #8699604 - Flags: review?(efaustbmo)
(Assignee)

Comment 4

2 years ago
Created attachment 8699605 [details]
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/
Attachment #8699605 - Flags: review?(benj)
(Assignee)

Comment 5

2 years ago
Created attachment 8699606 [details]
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/
Attachment #8699606 - Flags: review?(benj)
(Assignee)

Comment 6

2 years ago
Created attachment 8699607 [details]
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/
Attachment #8699607 - Flags: review?(benj)
(Assignee)

Comment 7

2 years ago
Created attachment 8699608 [details]
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/
Attachment #8699608 - Flags: review?(benj)
(Assignee)

Comment 8

2 years ago
Created attachment 8699609 [details]
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/
Attachment #8699609 - Flags: review?(benj)
(Assignee)

Comment 9

2 years ago
Created attachment 8699610 [details]
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/
Attachment #8699610 - Flags: review?(benj)

Updated

2 years ago
Attachment #8699604 - Flags: review?(efaustbmo) → review+
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.
Attachment #8699605 - Flags: review?(benj) → review+
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.
Attachment #8699606 - Flags: review?(benj) → review+
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 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
Attachment #8699607 - Flags: review?(benj) → review+
Attachment #8699608 - Flags: review?(benj) → review+
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.
Attachment #8699609 - Flags: review?(benj) → review+
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 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.
Attachment #8699610 - Flags: review?(benj) → review+
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
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.
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 23

2 years ago
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/
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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/
(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.
(Assignee)

Comment 28

2 years ago
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.
Depends on: 1235408
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.