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

SIMD: Implement .check in the interpreter and asm.js

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Not mandatory, but nice to have for being able to benchmark with the latest spec polyfill.
(Assignee)

Updated

3 years ago
Flags: needinfo?(benj)
(Assignee)

Comment 1

3 years ago
Created attachment 8563540 [details] [diff] [review]
1.check.intprtr.patch

This is for the interpreter...
Flags: needinfo?(benj)
Attachment #8563540 - Flags: review?(till)
(Assignee)

Comment 2

3 years ago
Created attachment 8563546 [details] [diff] [review]
2.check.asmjs.patch

Same thing in asm.js.  The idea behind this patch is to be able to have both kind of coercions for a short period of time, so that:
- emscripten can adjust (dan, let me know when you'd be ready for this change)
- we can adjust our benchmarks on awfy
- the Intel demo can be updated
Flags: needinfo?(sunfish)
Attachment #8563546 - Flags: review?(luke)
(Assignee)

Comment 3

3 years ago
Created attachment 8563547 [details] [diff] [review]
3.coercion.role.ctors.patch

And this patch effectively removes the constructor-as-a-coercion role, implying that all tests need to change.  A pretty big patch, but all tests are just adding check calls for coercions.  I am still running the test suite now and will add other test cases as I run into them.
Attachment #8563547 - Flags: review?(luke)
(Assignee)

Comment 4

3 years ago
Setting needinfo as a todo note for myself. Remaining parts:
- intel's github demo site
- awfy
- inline check in Ion
Flags: needinfo?(benj)

Comment 5

3 years ago
Comment on attachment 8563546 [details] [diff] [review]
2.check.asmjs.patch

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +2140,5 @@
> +    if (global->isSimdCtor() ||
> +        (global->isSimdOperation() && global->simdOperation() == AsmJSSimdOperation_check))
> +    {
> +        AsmJSSimdType type = global->isSimdCtor() ? global->simdCtorType()
> +                                                  : global->simdOperationType();

SM ternary style wants:
a
? b
: c
Attachment #8563546 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8563547 - Flags: review?(luke) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Created attachment 8563546 [details] [diff] [review]
> 2.check.asmjs.patch
> 
> Same thing in asm.js.  The idea behind this patch is to be able to have both
> kind of coercions for a short period of time, so that:
> - emscripten can adjust (dan, let me know when you'd be ready for this
> change)

The Emscripten changes will be easy. I'll update Emscripten whenever these patches land.
Flags: needinfo?(sunfish)
Attachment #8563540 - Flags: review?(till) → review?(evilpies)
Comment on attachment 8563540 [details] [diff] [review]
1.check.intprtr.patch

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

Inferring from the context this look fine.
Attachment #8563540 - Flags: review?(evilpies) → review+
(Assignee)

Comment 8

3 years ago
- code is ready on my fork of mandelbrot: https://github.com/bnjbvr/mandelbrot/tree/simd-check
- PR on awfy is ready: https://github.com/h4writer/arewefastyet/pull/47

Next step is to have trees open (at least to send to try and then ask checkin-needed). To be continued...
(Assignee)

Comment 9

3 years ago
Created attachment 8564229 [details] [diff] [review]
2.check.asmjs.patch

carrying forward r+ from luke
Attachment #8563546 - Attachment is obsolete: true
Attachment #8564229 - Flags: review+
(Assignee)

Comment 10

3 years ago
Requesting checking of patches 1 and 2 (if the try below is all green, or failures are unrelated. Related failures would be in ecma_7/SIMD js ref tests or asm.js/ jit-tests).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49f472bd1748
Keywords: checkin-needed
(Assignee)

Comment 11

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/099b350c49c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bd894fbfc8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/099b350c49c4
https://hg.mozilla.org/mozilla-central/rev/a1bd894fbfc8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 13

3 years ago
Sorry, forgot to put the leave-open keyword (see also comment 10 which was requesting landing of patches 1 and 2 only). Will land the last one asap.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> (see also comment 10 which was requesting landing of patches 1 and 2 only).

Bug marking is automated. Nobody saw it.
(Assignee)

Comment 15

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #14)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> > (see also comment 10 which was requesting landing of patches 1 and 2 only).
> 
> Bug marking is automated. Nobody saw it.

Correct me if I'm wrong, but I thought that when people are asking for checkin-needed, sheriffs would look at which patches they need to check-in? Or does it happen only when we put specific keywords in the keywords section / whiteboard?
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de196d11522

Dan, this change will trigger validation errors if the old coercion style (viz. not using check) is used. I've seen you've udpated emscripten this weekend (thank you for that). Just wanted to give you heads up, just in case you needed to activate something.
Flags: needinfo?(benj)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/5de196d11522
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.