Closed Bug 1153602 Opened 5 years ago Closed 4 years ago

SIMD (interpreter): Add more test cases for ReciprocalSqrtApproximation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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.
Depends on: 1150836
/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)
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)
Attachment #8591594 - Flags: review?(benj)
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 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)
Attachment #8591594 - Flags: review?(jwalden+bmo)
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 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+
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+
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+
Attachment #8591594 - Flags: review?(jwalden+bmo) → review+
Try push succeeded, adding checkin-needed.
Keywords: checkin-needed
Assignee: nobody → programfox
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/0d5e62c623f6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8591594 - Attachment is obsolete: true
Attachment #8620027 - Flags: review+
You need to log in before you can comment on or make changes to this bug.