Closed Bug 1094855 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(6 files, 1 obsolete file)

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+
This updates the min/max operations for float32x4, according to the assembly
sequences posted on the github repo.
Attachment #8522238 - Flags: review?(sunfish)
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)
Attached patch Add helpers for cmpps (obsolete) — Splinter Review
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)
(Previous patch was using ss suffix instead of ps suffix. Rather silly for
packed singles.)
Attachment #8522298 - Flags: review?(sunfish)
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+
Attachment #8522240 - Flags: review?(sunfish) → review+
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)
Annnnnnnnd that will be done.
Attachment #8522381 - Flags: review?(luke)
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+
Attachment #8522381 - Flags: review?(luke) → review+
You need to log in before you can comment on or make changes to this bug.