Closed
Bug 1113445
Opened 6 years ago
Closed 6 years ago
SIMD: handle -0 properly in signMask
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: sunfish, Assigned: sunfish)
Details
Attachments
(1 file, 1 obsolete file)
6.23 KB,
patch
|
bbouvier
:
review+
Waldo
:
review+
|
Details | Diff | Splinter Review |
The SIMD.js signMask operation is defined to examine the sign bit of each value. -0 has a non-zero sign bit, but -0 < 0 is false, so a floating-point comparison isn't sufficient. The attached patch implements correct handling on -0, and adds tests.
Attachment #8538913 -
Flags: review?(benj)
Comment 1•6 years ago
|
||
Comment on attachment 8538913 [details] [diff] [review] signmask-negative-zero.patch Review of attachment 8538913 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch, thanks! It will even scale nicely for Float64x2, but probably less nicely for int8x16 / int16x8... ::: js/src/tests/ecma_7/SIMD/float32x4signmask.js @@ +1,2 @@ > +// |reftest| skip-if(!this.hasOwnProperty("SIMD")) > +var BUGNUMBER = 946042; You can remove BUGNUMBER and any reference to it in this file. @@ +1,4 @@ > +// |reftest| skip-if(!this.hasOwnProperty("SIMD")) > +var BUGNUMBER = 946042; > + > +var float32x4 = SIMD.float32x4; Can you add tests for int32x4, and rename the file signmask.js, please? @@ +5,5 @@ > + > +function test() { > + print(BUGNUMBER + ": " + summary); > + > + for ([v, w] of [[float32x4(-1, 20, 30, 4), 0b0001], Technically, v and w should be declared somewhere, otherwise they'll be considered as globals and thus would get the GETGLOBAL and other deoptimized JSOP. Can you please declare them above the loop?
Attachment #8538913 -
Flags: review?(benj) → review+
Assignee | ||
Comment 2•6 years ago
|
||
Thanks. Here's a patch which should handle arbitrary SIMD types correctly, and which addresses the other review comments.
Attachment #8538913 -
Attachment is obsolete: true
Attachment #8539323 -
Flags: review?(benj)
Comment 3•6 years ago
|
||
Comment on attachment 8539323 [details] [diff] [review] signmask-negative-zero.patch Review of attachment 8539323 [details] [diff] [review]: ----------------------------------------------------------------- Wow, even better, thanks! Waldo, can you review changes made in mfbt/integerTypeTraits.h, please? ::: js/src/builtin/SIMD.cpp @@ +12,5 @@ > */ > > #include "builtin/SIMD.h" > > +#include "mozilla/IntegerTypeTraits.h" Beware the style check: this include should be after js/Value.h, I think. To be tested with make check-style in the build dir, iirc. @@ +151,5 @@ > + // Load the data as integer so that we treat the sign bit consistently, > + // since -0.0 is not less than zero, but it still has the sign bit set. > + typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int; > + const uint8_t *data = typedObj.typedMem(); > + int32_t result = 0; To be sure we don't read values out of limits, can we assert: MOZ_ASSERT(SimdType::lanes * sizeof(Int) < jit::Simd128DataSize); @@ +156,5 @@ > + for (unsigned i = 0; i < SimdType::lanes; ++i) { > + Int x; > + memcpy(&x, &data[i * sizeof(Int)], sizeof(Int)); > + result |= (x < 0) << i; > + } nice! ::: mfbt/IntegerTypeTraits.h @@ +81,5 @@ > > +template<size_t Size> > +struct SignedStdintTypeForSize > + : detail::StdintTypeForSizeAndSignedness<Size, true> > +{}; I am not a peer of mfbt, so might as well ask Waldo if that's fine to add.
Attachment #8539323 -
Flags: review?(jwalden+bmo)
Attachment #8539323 -
Flags: review?(benj)
Attachment #8539323 -
Flags: review+
Comment 4•6 years ago
|
||
Comment on attachment 8539323 [details] [diff] [review] signmask-negative-zero.patch Review of attachment 8539323 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SIMD.cpp @@ +150,5 @@ > > + // Load the data as integer so that we treat the sign bit consistently, > + // since -0.0 is not less than zero, but it still has the sign bit set. > + typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int; > + const uint8_t *data = typedObj.typedMem(); const Elem *elems = reinterpret_cast<const Elem*>(typedObj.typedMem()); @@ +151,5 @@ > + // Load the data as integer so that we treat the sign bit consistently, > + // since -0.0 is not less than zero, but it still has the sign bit set. > + typedef typename mozilla::SignedStdintTypeForSize<sizeof(Elem)>::Type Int; > + const uint8_t *data = typedObj.typedMem(); > + int32_t result = 0; The suggested assertion can be a static_assert, right? @@ +154,5 @@ > + const uint8_t *data = typedObj.typedMem(); > + int32_t result = 0; > + for (unsigned i = 0; i < SimdType::lanes; ++i) { > + Int x; > + memcpy(&x, &data[i * sizeof(Int)], sizeof(Int)); Int x = mozilla::BitwiseCast<Int>(elems[i]);
Attachment #8539323 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 5•6 years ago
|
||
Comments addressed. Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/25507fee09eb
Comment 6•6 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=145cfaf3c177&tochange=2f90deb1be15 appears to have caused a 3.7% regression on AWFY-Octane-EarlyBoyer on machine 28 http://arewefastyet.com/#machine=28&view=breakdown&suite=octane
Comment 7•6 years ago
|
||
(In reply to mayankleoboy1 from comment #6) > http://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?fromchange=145cfaf3c177&tochange=2f90deb1be15 > appears to have caused a 3.7% regression on AWFY-Octane-EarlyBoyer on > machine 28 http://arewefastyet.com/#machine=28&view=breakdown&suite=octane It's more likely that it's due to bug 1096138, which I don't have access to. Opened bug 1115330 for tracking.
Comment 8•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/25507fee09eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•