Closed
Bug 1153602
Opened 5 years ago
Closed 5 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.458340731200208e155 > js> Math.sqrt(1 / Number.MAX_VALUE) // exponential stringification ends in 7 > 7.458340731200207e155 > 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•5 years ago


/r/6993  Bug 1153602  SIMD (interpreter): Added more test cases for ReciprocalSqrtApproximation Pull down this commit: hg pull r eea76efd6821f46f0902aa948fd3f6c0fd62b0d0 https://reviewboardhg.mozilla.org/gecko/
Attachment #8591594 
Flags: review?(benj)
Comment 2•5 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•5 years ago

Attachment #8591594 
Flags: review?(benj)
Assignee  
Comment 3•5 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://reviewboardhg.mozilla.org/gecko/
Attachment #8591594 
Flags: review?(jwalden+bmo)
Comment 4•5 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/float64x2arithmetic.js (Diff revision 2) > + [float64x2(INT32_MIN, INT32_MAX), float64x2(Number.EPSILON, 0.5)]]) These values seem nonresponsive 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 tooprecise and round wrong. Or am I missing something here? Fairly sure I'm not.
Attachment #8591594 
Flags: review?(jwalden+bmo)
Assignee  
Updated•5 years ago

Attachment #8591594 
Flags: review?(jwalden+bmo)
Assignee  
Comment 5•5 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://reviewboardhg.mozilla.org/gecko/
Comment 6•5 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/unaryoperations.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/unaryoperations.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 asis 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•5 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•5 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://reviewboardhg.mozilla.org/gecko/
Attachment #8591594 
Flags: review?(jwalden+bmo)
Attachment #8591594 
Flags: review+
Assignee  
Updated•5 years ago

Attachment #8591594 
Flags: review?(jwalden+bmo) → review+
Assignee  
Updated•5 years ago

Assignee: nobody → programfox
Status: NEW → ASSIGNED
Comment 10•5 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/0d5e62c623f6
Keywords: checkinneeded
Comment 11•5 years ago


https://hg.mozilla.org/mozillacentral/rev/0d5e62c623f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution:  → FIXED
Target Milestone:  → mozilla40
Assignee  
Comment 12•5 years ago


Attachment #8591594 
Attachment is obsolete: true
Attachment #8620027 
Flags: review+
Assignee  
Comment 13•5 years ago


You need to log in
before you can comment on or make changes to this bug.
Description
•