Closed Bug 1201934 Opened 4 years ago Closed 4 years ago

Remove SIMD shiftRightArithmeticByScalar and shiftRightLogicalByScalar

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jujjyl, Assigned: jolesen)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file)

SIMD.js recently renamed the function in the spec, see https://github.com/tc39/ecmascript_simd/commit/97215fd3824168f8dd998b7f2d1a1e0bbcd0cc57 . Please rename the function in the SpiderMonkey implementation, so that SpiderMonkey will validate as asm.js in its SIMD.js support.

See https://github.com/juj/emscripten/commit/629e7f8675d232911cc28516577ed0c25345980d and https://github.com/juj/emscripten/commit/629e7f8675d232911cc28516577ed0c25345980d and https://github.com/kripken/emscripten-fastcomp/pull/117.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
I postponed the change to rename these functions in Emscripten. Also, I notice that the function shiftRightArithmeticByScalar was also renamed to shiftRightByScalar in the spec, and the Logical vs Arithmetic is now determined by the SIMD type.
I think it would make sense to make this change as part of Bug 1233111 which introduces the unsigned types.
Depends on: 1233111
To clarify, bug 1233111 will add a shiftRightByScalar function on all of the integer SIMD types. It will leave in place the shiftRightArithmeticByScalar and shiftRightLogicalByScalar functions.

I will use this bug to remove shiftRightArithmeticByScalar and shiftRightLogicalByScalar after bug 1233111 has landed.
Assignee: nobody → jolesen
Summary: Rename SIMD.js shiftRightLogicalByScalar to shiftRightByScalar. → Remove SIMD shiftRightArithmeticByScalar and shiftRightLogicalByScalar
Blocks: 894105
Emscripten has now been adapted to not use the shiftRightArithmetic/LogicalByScalar, but only uses the shiftRightByScalar version, so it's not referring to the old spec anymore. See https://github.com/kripken/emscripten/pull/4066
These functions have been replaced by the shiftRightByScalar function which
behaves differently on signed and unsigned SIMD types.

This patch applies on top of the patches from bug 1244889.
Attachment #8718498 - Flags: review?(sunfish)
Depends on: 1244889
Comment on attachment 8718498 [details] [diff] [review]
Remove SIMD shiftRight***ByScalar.

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

::: js/src/jit-test/tests/SIMD/shift.js
@@ -41,5 @@
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, 0),  a.map(ursh(0)));
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, 1),  a.map(ursh(1)));
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, 2),  a.map(ursh(2)));
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, 31), a.map(ursh(31)));
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, 32), a.map(ursh(32)));

Could we keep these tests, using SIMD.Uint32x4.fromInt32x4Bits(v).shiftRightByScalar? AFAIK this is the only non-asm.js coverage of these operations in jit-test.

@@ -54,5 @@
>          // Non constant shift counts
>          var c = shifts[i % shifts.length];
>          assertEqX4(SIMD.Int32x4.shiftLeftByScalar(v, c), a.map(lsh(c)));
> -        assertEqX4(SIMD.Int32x4.shiftRightArithmeticByScalar(v, c), a.map(rsh(c)));
> -        assertEqX4(SIMD.Int32x4.shiftRightLogicalByScalar(v, c), a.map(ursh(c)));

Ditto :).
Attachment #8718498 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/0af8ffcf6870
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.