Closed
Bug 1153602
Opened 9 years ago
Closed 9 years ago
SIMD (interpreter): Add more test cases for ReciprocalSqrtApproximation
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: ProgramFOX, Assigned: ProgramFOX)
References
Details
Attachments
(1 file, 1 obsolete file)
Quoting Waldo's comment at bug 1150836 comment 11: > I don't doubt these were, those are easy/obvious cases. :-) > But were there any tests whose behavior, outside these obvious cases, > would *change* with this semantic change? Tacking together multiple > operations often can induce loss of precision. > For example, DBL_MAX passed into the semantics gives different values > because of loss of precision: > js> (1 / Math.sqrt(Number.MAX_VALUE)) // exponential stringification ends in 8 > 7.458340731200208e-155 > js> Math.sqrt(1 / Number.MAX_VALUE) // exponential stringification ends in 7 > 7.458340731200207e-155 > js> (1 / Math.sqrt(Number.MAX_VALUE)) === Math.sqrt(1 / Number.MAX_VALUE) > false > It seems highly desirable to have a few tests along these lines, for float32 and float64 both.
Assignee | ||
Comment 1•9 years ago
|
||
/r/6993 - Bug 1153602 - SIMD (interpreter): Added more test cases for ReciprocalSqrtApproximation Pull down this commit: hg pull -r eea76efd6821f46f0902aa948fd3f6c0fd62b0d0 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8591594 -
Flags: review?(benj)
Comment 2•9 years ago
|
||
https://reviewboard.mozilla.org/r/6993/#review5811 Values need to be changed for Float32x4, as the implicit float truncation makes them not so interesting. On your next version, can you please request review from Waldo? ::: js/src/tests/ecma_7/SIMD/float32x4reciprocalsqrt.js (Diff revision 1) > + var j = float32x4(Number.MAX_VALUE, Number.MIN_VALUE, Number.EPSILON, -1); Math.fround(Number.MAX_VALUE) === +Infinity Math.fround(Number.MIN_VALUE) === 0 I think you need to use other values here: Math.pow(2, 31) Math.pow(2, -31)
Updated•9 years ago
|
Attachment #8591594 -
Flags: review?(benj)
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX /r/6993 - Bug 1153602 - SIMD (interpreter): Added more test cases for ReciprocalSqrtApproximation Pull down this commit: hg pull -r 1e1756edf4ef6523aba4746a596c57007146dde2 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8591594 -
Flags: review?(jwalden+bmo)
Comment 4•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX https://reviewboard.mozilla.org/r/6991/#review5941 ::: js/src/tests/ecma_7/SIMD/float64x2-arithmetic.js (Diff revision 2) > + [float64x2(INT32_MIN, INT32_MAX), float64x2(Number.EPSILON, -0.5)]]) These values seem non-responsive to #c0's point. INT32_MIN and -0.5 go to NaN trivially (no harm testing them, tho). But INT32_MAX and Number.EPSILON don't trigger imprecision: js> var oneOverSqrt = n => 1 / Math.sqrt(n); js> var sqrtOneOver = n => Math.sqrt(1 / n); js> oneOverSqrt(Math.pow(2, 31) - 1) === sqrtOneOver(Math.pow(2, 31) - 1) true js> oneOverSqrt(Number.EPSILON) === sqrtOneOver(Number.EPSILON) true No harm testing these, either: but no help as far as #c0 goes. Number.MAX_VALUE would work for testing, but it's finicky to exactly write, because it has so many bits set. Try Number.MIN_VALUE (2\*\*-1074) instead: js> 1 / Math.sqrt(Math.pow(2, -1074)) === Math.sqrt(1 / Math.pow(2, -1074)) false js> 1 / Math.sqrt(Math.pow(2, -1074)) 4.4989137945431964e+161 js> Math.sqrt(1 / Math.pow(2, -1074)) Infinity ::: js/src/tests/ecma_7/SIMD/float32x4reciprocalsqrt.js (Diff revision 2) > + var j = float32x4(INT32_MAX, INT32_MIN, Number.EPSILON, -1); This has the same problems as the double version -- INT32_MAX/Number.EPSILON won't trigger imprecision. FLT_MIN, i.e. 2\*\*-149, should get the job done again. But, I happened to notice: we currently have this definition for the emulation to test against: \+ function reciprocalsqrtf(a) \{ \+ return Math.fround(1 / Math.sqrt(a)); \+ \} This isn't right\! We should be frounding |a|, if we're not requiring it to be a float32 on entry. (And if we are, we should assert that.) And we should be frounding the result of the Math.sqrt, because that could easily be too-precise and round wrong. Or am I missing something here? Fairly sure I'm not.
Attachment #8591594 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8591594 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX /r/6993 - Bug 1153602 - SIMD (interpreter): Added more test cases for ReciprocalSqrtApproximation Pull down this commit: hg pull -r ada3281a69501af8cec76e50e1e5a276ad9e57e6 https://reviewboard-hg.mozilla.org/gecko/
Comment 6•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX https://reviewboard.mozilla.org/r/6991/#review6243 ::: js/src/tests/ecma_7/SIMD/unary-operations.js:54 (Diff revision 3) > - return Math.fround(1 / Math.sqrt(a)); > + return Math.fround(1 / Math.fround(Math.sqrt(Math.fround(a)))); It looks like we can ensure only float32 values flow in here, so instead of |Math.fround(a)|, use |a| directly and add assertEq(Math.fround(a), a); as a sanity check. ::: js/src/tests/ecma_7/SIMD/unary-operations.js:62 (Diff revision 3) > + [Math.pow(2, 31), Math.pow(2, -31), Math.pow(2, -1074), Math.pow(2, -149)].map(reciprocalsqrtf)] If you're passing these into float32 values, Math.pow(2, -1074) isn't a float32 and will round to +0. There's no reason not to use +0 directly. (And certainly +0 is a very good candidate for testing.) Past that, on second glance it seems very, very strange to me to use reciprocalsqrtf to generate expected values here. I'd perform comparisons against constants, or arithmetic expressions as close to constant as possible, so that there's less of a chance of offsetting errors: var vals = [ [[1, 1, 0.25, 0.25], [1, 1, 2, 2]], [[25, 16, 6.25, 1.5625], [Math.fround(1 / 5), 0.25, Math.fround(1 / 2.5), Math.fround(1 / 1.25)]], [[NaN, -0, Infinity, -Infinity], [NaN, -Infinity, +0, NaN]], [[Math.pow(2, 32), Math.pow(2, -32), +0, Math.pow(2, -148)], [Math.pow(2, -16), Math.pow(2, 16), Infinity, Math.pow(2, 74)]], ]; I checked, and 2**-148 has the same 1/sqrt and sqrt(1/) differences as that was designed to test, originally. But 2**-148 and 2**32 are better here because the results of the squaring aren't irrational. Probably there *should* be a test with an irrational expected output, but I've been staring at this too long as-is and shouldn't take more time to come up with a nice testcase. If you end up coming up with one, please write out its value as an exact binary fraction in decimal form --
Attachment #8591594 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX https://reviewboard.mozilla.org/r/6991/#review6629 Patch updated. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a31f0abdd63
Attachment #8591594 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8591594 [details] MozReview Request: bz://1153602/ProgramFOX /r/6993 - Bug 1153602 - SIMD (interpreter): Added more test cases for ReciprocalSqrtApproximation. r=Waldo Pull down this commit: hg pull -r 86eaa6619eeed9a995b09074643ec84a32e60f61 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8591594 -
Flags: review?(jwalden+bmo)
Attachment #8591594 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8591594 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → programfox
Status: NEW → ASSIGNED
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5e62c623f6
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d5e62c623f6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8591594 -
Attachment is obsolete: true
Attachment #8620027 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•