Closed
Bug 1246800
Opened 8 years ago
Closed 8 years ago
SIMD: Implement and test masked shift-by-scalar amounts
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: jolesen, Assigned: jolesen)
Details
Attachments
(1 file, 1 obsolete file)
17.17 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → jolesen
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=346133511ce5
Comment 3•8 years ago
|
||
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•8 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.
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd3508bcede4
Assignee | ||
Updated•8 years ago
|
Attachment #8721047 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8721366 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaec1c77ae38d071284c61294ea70a59a3b7e09 Bug 1246800 - Masked shift-by-scalar amounts. r=sunfish
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cfaec1c77ae3
Status: NEW → RESOLVED
Closed: 8 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.
Description
•