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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Not mandatory, but nice to have for being able to benchmark with the latest spec polyfill.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.check.intprtr.patch

This is for the interpreter...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Setting needinfo as a todo note for myself. Remaining parts:
- intel's github demo site
- awfy
- inline check in Ion

Comment 5

[Luke Wagner [:luke]]

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

Comment 6

[Dan Gohman [:sunfish]]

(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.

Comment 7

[Tom S [:evilpie]]

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.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

- 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...

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.check.asmjs.patch

carrying forward r+ from luke

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/099b350c49c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bd894fbfc8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/099b350c49c4
https://hg.mozilla.org/mozilla-central/rev/a1bd894fbfc8

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

(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?

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5de196d11522