SIMD: Implement and test masked shift-by-scalar amounts

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jolesen, Assigned: jolesen)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The SIMD.js functions for shifting a vector by a scalar are now masking the scalar to the size if the vector lanes: https://github.com/tc39/ecmascript_simd/pull/320

Make sure that we implement these semantics, and that we have tests.
(Assignee)

Comment 1

2 years ago
Created attachment 8721047 [details] [diff] [review]
Masked shift-by-scalar amounts.

Modulo-reduce SIMD shift amounts by the size of the lane instead of saturating
shift amounts larger than the number of bits in a lane.
Attachment #8721047 - Flags: review?(sunfish)
(Assignee)

Updated

2 years ago
Assignee: nobody → jolesen
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=346133511ce5
Comment on attachment 8721047 [details] [diff] [review]
Masked shift-by-scalar amounts.

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

Looks good!

::: js/src/builtin/SIMD.cpp
@@ +718,5 @@
>  template<typename T>
>  struct ShiftLeft {
>      static T apply(T v, int32_t bits) {
> +        uint32_t maskedBits = uint32_t(bits) % (sizeof(T) * 8);
> +        return  v << maskedBits;

nit: extra space after `return`.

Also, C++'s left shift on signed types has a bunch of undefined behavior, so this should cast v to MakeUnsigned<T>::Type before doing the shift.

@@ +733,5 @@
>  template<typename T>
>  struct ShiftRightLogical {
>      static T apply(T v, int32_t bits) {
> +        uint32_t maskedBits = uint32_t(bits) % (sizeof(T) * 8);
> +        return uint32_t(v) >> maskedBits;

For consistency, and correctness if this code ever gets used for 64-bit shifts, it'd be good to use MakeUnsigned<T>::Type instead of uint32_t for this shift too.
Attachment #8721047 - Flags: review?(sunfish) → review+
(Assignee)

Comment 4

2 years ago
Ugh, there is no ScratchRegisterScope on 32-bit x86. I'll need to insert that masking `and` instruction before register allocation. I guess LIRGenerator::visitSimdShift() can do it.
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #4)
> Ugh, there is no ScratchRegisterScope on 32-bit x86. I'll need to insert
> that masking `and` instruction before register allocation. I guess
> LIRGenerator::visitSimdShift() can do it.

Just curious, we can't add a temp register to LSimdShift (and LDefinition::BogusTemp() when we know we don't need it)?
(Assignee)

Comment 6

2 years ago
Created attachment 8721366 [details] [diff] [review]
Masked shift-by-scalar amounts.

Dan, would you mind giving this one a second look?

I used MakeUnsigned<T> as you suggested to avoid undefined behavior in the
left-shifting function.

I added a temp register to LSimdShift as Jan suggested. It seems to work fine,
but please double-check.

Everything else is unchanged.
Attachment #8721366 - Flags: review?(sunfish)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3508bcede4
(Assignee)

Updated

2 years ago
Attachment #8721047 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8721366 - Flags: review?(sunfish) → review+
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaec1c77ae38d071284c61294ea70a59a3b7e09
Bug 1246800 - Masked shift-by-scalar amounts. r=sunfish

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cfaec1c77ae3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.