If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SIMD (interpreter): change order of operations of ReciprocalSqrtApproximation

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ProgramFOX, Assigned: ProgramFOX)

Tracking

Trunk
mozilla40
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
https://github.com/johnmccutchan/ecmascript_simd/commit/bd5485311009cdc58e5ef13ea523925f0b81f6cb

The order of operations in the spec changed from `sqrt(1 / x)` to `1 / sqrt(x)`.
(Assignee)

Comment 1

3 years ago
Created attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX

/r/6591 - Bug 1150836 - SIMD (interpreter): change order of operations of ReciprocalSqrtApproximation

Pull down this commit:

hg pull -r af9cbcdabfba9b08099fcea7f28f8fe8677f1f37 https://reviewboard-hg.mozilla.org/gecko/
(Assignee)

Comment 2

3 years ago
Comment on attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX

/r/6591 - Bug 1150836 - SIMD (interpreter): change order of operations of ReciprocalSqrtApproximation

Pull down this commit:

hg pull -r af9cbcdabfba9b08099fcea7f28f8fe8677f1f37 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8587922 - Flags: review?(benj)
Comment on attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX

https://reviewboard.mozilla.org/r/6589/#review5567

Thanks for doing this! Feel free to add a few tests in float32x4reciprocalsqrt.js regarding special values (NaN, +/-Infinity, -0, etc.).
Attachment #8587922 - Flags: review?(benj) → review+
(Assignee)

Comment 4

3 years ago
Comment on attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX

https://reviewboard.mozilla.org/r/6589/#review5621
Attachment #8587922 - Flags: review+
(Assignee)

Comment 5

3 years ago
Comment on attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX

/r/6591 - Bug 1150836 - SIMD (interpreter): change order of operations of ReciprocalSqrtApproximation. r=bbouvier

Pull down this commit:

hg pull -r 3240f9d5d0e8f3076a1ef851ccd1b4c17a33d912 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8587922 - Flags: review?(benj)
Attachment #8587922 - Flags: review+
(Assignee)

Comment 6

3 years ago
https://reviewboard.mozilla.org/r/6591/#review5623

Ship It!
(Assignee)

Updated

3 years ago
Attachment #8587922 - Flags: review?(benj) → review+
(Assignee)

Comment 7

3 years ago
> Feel free to add a few tests in float32x4reciprocalsqrt.js regarding special values (NaN, +/-Infinity, -0, etc.).

These already were there, so I did not have to make any changes.

(I was not entirely sure how to r+ my own patch on Reviewboard, I hope I did it right here)

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=424c18b72b7b
That's great like this, thank you! There is no proper way yet to support carrying forward r+ on reviewboard at the moment...
Assignee: nobody → programfox
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 years ago
Try push succeeded, adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d09658bd866
Flags: in-testsuite+
Keywords: checkin-needed
(In reply to :ProgramFOX from comment #7)
> > Feel free to add a few tests in float32x4reciprocalsqrt.js regarding special values (NaN, +/-Infinity, -0, etc.).
> 
> These already were there, so I did not have to make any changes.

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 8
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.
...er, ends in *7* for the second example.  Copypasta.
(Assignee)

Updated

3 years ago
Blocks: 1153602
(Assignee)

Comment 13

3 years ago
> It seems highly desirable to have a few tests along these lines, for float32 and float64 both.

Sure. Because the patch for this bug is already checked in on inbound, I opened a follow-up bug for this, bug 1153602.
https://hg.mozilla.org/mozilla-central/rev/4d09658bd866
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 15

2 years ago
Comment on attachment 8587922 [details]
MozReview Request: bz://1150836/ProgramFOX
Attachment #8587922 - Attachment is obsolete: true
Attachment #8619962 - Flags: review+
(Assignee)

Comment 16

2 years ago
Created attachment 8619962 [details]
MozReview Request: Bug 1150836 - SIMD (interpreter): change order of operations of ReciprocalSqrtApproximation. r=bbouvier
You need to log in before you can comment on or make changes to this bug.