Closed Bug 1141629 Opened 5 years ago Closed 5 years ago

SIMD: Clarify that reciprocal and reciprocalSqrt are approximations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ProgramFOX, Assigned: ProgramFOX)

Details

Attachments

(1 file, 2 obsolete files)

https://github.com/johnmccutchan/ecmascript_simd/commit/ff6056db82c9f151a7a73866d28062ecf6903c72
The reciprocal and reciprocalSqrt methods will have to be renamed to reciprocalApproximation and reciprocalSqrtApproximation.
Proposed patch for this bug.
Attachment #8575491 - Flags: review?(benj)
Comment on attachment 8575491 [details] [diff] [review]
Clarify that reciprocal and reciprocalSqrt are approximations

Review of attachment 8575491 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! Please make sure that Dan can have a chance to update emscripten before you push this :)

::: js/src/jit-test/tests/asm.js/testSIMD.js
@@ +565,4 @@
>  CheckRecp([1, 42.42, 0.63, 13.37]);
>  CheckRecp([NaN, -Infinity, Infinity, 0]);
>  
> +var CheckRecp = CheckUnaryF4('reciprocalSqrtApproximation', function(x) { return 1 / Math.sqrt(x); }, assertNear);

While you're here, can you check if the test still passes with Math.sqrt(1 / x) instead, please?

::: js/src/jit/MIR.h
@@ +1967,5 @@
>            case abs:            return "abs";
>            case neg:            return "neg";
>            case not_:           return "not";
> +          case reciprocalApproximation:     return "reciprocalApproximation";
> +          case reciprocalSqrtApproximation: return "reciprocalSqrtApproximation";

nit: please align all return (if you use vim, there's an extension for that called tabularize)

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +960,5 @@
>      }
>      void packedSubInt32(const Operand &src, FloatRegister dest) {
>          vpsubd(src, dest, dest);
>      }
> +    void packedReciprocalApproximationFloat32x4(const Operand &src, FloatRegister dest) {

I am not a big fan of the long name. Can you make this one (and the one below) shorter by replacing Reciprocal by Rcp, please? (we're already using sqrt below, so that sounds fine)
Attachment #8575491 - Flags: review?(benj) → review+
cc'ing Dan so that he's aware we're changing the name, and can get a chance to update emscripten :)
Go ahead and check this in whenever you're ready. I'll update emscripten once it's in.
> While you're here, can you check if the test still passes with Math.sqrt(1 / x) instead, please?

No, the test doesn't pass when doing that.

Should the test stay as it was, or should the C++ code be adjusted to make it pass?
(In reply to ProgramFOX from comment #5)
> > While you're here, can you check if the test still passes with Math.sqrt(1 / x) instead, please?
> 
> No, the test doesn't pass when doing that.
> 
> Should the test stay as it was, or should the C++ code be adjusted to make
> it pass?

The test should stay as is. When looking at the Intel manual, it seems that this time, this is the polyfill which does things in the wrong order. Opened https://github.com/johnmccutchan/ecmascript_simd/issues/121
Attached patch rec-approx2.diff (obsolete) — Splinter Review
Updated patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c900741394b
Assignee: nobody → programfox
Attachment #8575491 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8576723 - Flags: review+
Some builds got busted on the Try push even after a retrigger and I couldn't find an error message related to this patch, so I pulled to be sure I was on a fresh clone, refreshed the patch and the patch containing the try syntax, and I pushed again. This attachment contains the new patch (the only thing that's updated is the Parent Node).

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9719029fb846
Attachment #8576723 - Attachment is obsolete: true
Attachment #8577347 - Flags: review+
The new try push returned all green, adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/672a7ade30a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.