SIMD.js: Add minNum/maxNum to the interpreter and update min/max

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
See [0] for the reference impl.

[0] https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js
(Assignee)

Comment 1

3 years ago
Created attachment 8518194 [details] [diff] [review]
SIMD: Add minNum/maxNum and update min/max in the interpreter

Should be straightforward. Contemplating the tests makes me think we could
probably self-host the entire SIMD implementation...
Attachment #8518194 - Flags: review?(till)
Comment on attachment 8518194 [details] [diff] [review]
SIMD: Add minNum/maxNum and update min/max in the interpreter

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

Yes, this works.

Or, you could just self-host these SIMD functions *and* Math.min/max. I'd assume that you'd still want Ion fast-paths as these operations can probably be turned into better specific instructions. That's entirely possible to do with self-hosted versions, and would speed up baseline, IIUC.
Attachment #8518194 - Flags: review?(till) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8522238 [details] [diff] [review]
Update SimdBinaryArith::Min/Max to properly handle comparisons involving -0/0 and NaNs

This updates the min/max operations for float32x4, according to the assembly
sequences posted on the github repo.
Attachment #8522238 - Flags: review?(sunfish)
(Assignee)

Comment 4

3 years ago
Created attachment 8522240 [details] [diff] [review]
Move some tests in testSIMD.js

Not sure this even needs a review, as it just moves patches, but anyways. This
moves tests that were supposed to stay close to multiply/divide.
Attachment #8522240 - Flags: review?(sunfish)
(Assignee)

Comment 5

3 years ago
Created attachment 8522243 [details] [diff] [review]
Add helpers for cmpps

This adds helpers for using cmpps and not having these weird const integers
show up at various places in the code, plus it's more consistent with x86's
doc.
Attachment #8522243 - Flags: review?(sunfish)
(Assignee)

Comment 6

3 years ago
Created attachment 8522298 [details] [diff] [review]
Add helpers for cmpps

(Previous patch was using ss suffix instead of ps suffix. Rather silly for
packed singles.)
Attachment #8522298 - Flags: review?(sunfish)
(Assignee)

Updated

3 years ago
Attachment #8522243 - Attachment is obsolete: true
Attachment #8522243 - Flags: review?(sunfish)
Comment on attachment 8522238 [details] [diff] [review]
Update SimdBinaryArith::Min/Max to properly handle comparisons involving -0/0 and NaNs

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

Looks good!
Comment on attachment 8522238 [details] [diff] [review]
Update SimdBinaryArith::Min/Max to properly handle comparisons involving -0/0 and NaNs

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

Looks good!
Attachment #8522238 - Flags: review?(sunfish) → review+

Updated

3 years ago
Attachment #8522240 - Flags: review?(sunfish) → review+
(Assignee)

Comment 9

3 years ago
Created attachment 8522379 [details] [diff] [review]
Implement SIMD.float32x4.minNum/maxNum in the JITs

Oh boy, even with pseudo code in front of the eyes, this gives headache to
implement without blendvps and avx forms. Bleh.
Attachment #8522379 - Flags: review?(sunfish)
(Assignee)

Comment 10

3 years ago
Created attachment 8522381 [details] [diff] [review]
Add SIMD.float32x4.minNum/maxNum to asm.js

Annnnnnnnd that will be done.
Attachment #8522381 - Flags: review?(luke)
(Assignee)

Updated

3 years ago
Blocks: 1068028
Comment on attachment 8522298 [details] [diff] [review]
Add helpers for cmpps

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

Please define an enum in BaseAssembler-x86-shared.h for the actual constants, similar to the Condition enum.

That will make it less important to make the cmpps interface private, but I'm ok either way. I do like it that the code now uses cmpltps and friends.
Attachment #8522298 - Flags: review?(sunfish) → review+
Comment on attachment 8522379 [details] [diff] [review]
Implement SIMD.float32x4.minNum/maxNum in the JITs

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

Looks good.
Attachment #8522379 - Flags: review?(sunfish) → review+

Updated

3 years ago
Attachment #8522381 - Flags: review?(luke) → review+
(Assignee)

Comment 13

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e21104ba9205
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cecc072d44bc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8937c8785f74
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd252c4cfad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ba8aa473d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd68fda05a2b
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e21104ba9205
https://hg.mozilla.org/mozilla-central/rev/cecc072d44bc
https://hg.mozilla.org/mozilla-central/rev/8937c8785f74
https://hg.mozilla.org/mozilla-central/rev/dcd252c4cfad
https://hg.mozilla.org/mozilla-central/rev/f7ba8aa473d3
https://hg.mozilla.org/mozilla-central/rev/dd68fda05a2b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.