1063946 - SIMD (interpreter): Group tests in logical units

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:ProgramFOX]

In the SIMD test cases (/js/src/tests/ecma_6/TypedObject/simd), there is an unused variable in most files, for example:
> var float32x4 = SIMD.float32x4;
In an int32x4 test case where float32x4 is not used (or vice versa). As creating these variables is useless, they can be removed.

And in some test cases, the summary (stored in the summary variable in the test case) is incorrect (such as 'int32x4 with' in int32x4withflag.js, then the summary should be changed in 'int32x4 withFlag'

Comment 1

[:ProgramFOX]

At the moment, this bug depends on bug 948379 because there is a patch there that will be landed soon (which edits the same files), so actually you can only start working on this after that bug is fixed. I'll post a comment here when that patch is landed.

Comment 2

[:ProgramFOX]

In comment 1, I meant "when that patch is landed" instead of "after that bug is fixed".

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for filing this bug, ProgramFOX!

Comment 4

[Aman Singh]

I want to work on this bug. Please help me.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Aman Singh from comment #4)
> I want to work on this bug. Please help me.

Thanks for jumping into this, Aman! have you managed to build the JS shell, as described in [1]? Once that is done, you should try to run tests, as described in this same page. Afterwards, you can find the source of the above mentioned source files in $base/js/src/tests/ecma_6/TypedObject/simd. You can then submit a patch using mercurial [2] to this bug.

Feel free to ask more questions here if you are getting stuck at any point, or to jump in the #jsapi channel on irc.mozilla.org. Cheers!

[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation
[2] https://developer.mozilla.org/en-US/docs/Mercurial

Comment 6

[:ProgramFOX]

This bug no longer depends on bug 948379 as the int32x4 border cases have been landed now.

You might need to pull and update again to make sure that your repository contains the changes of that patch. It is not hard to check: go to the int32x4add.js test case and check whether there is an INT32_MAX and INT32_MIN variable. If there is, you don't have to pull and update. If there is not, run hg pull and hg update to update your repository.

Comment 7

[:ProgramFOX]

Hey Aman,

Any news about this? Are you making progress on the bug?

Comment 8

[Aman Singh]

Yes I was busy with my exams but I have started working on this.

Comment 9

[Aman Singh]

Attachment: Bug 1063946.patch

Please review the Patch.

Comment 10

[:ProgramFOX]

Comment on attachment 8490662 [details] [diff] [review]
Bug 1063946.patch

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

First of all, thanks a lot for working on this bug! I have been looking at the patch, and I have seen that some variables are removed where they were used. Please see my comments below.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4alignment.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 938728;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the summary and the float32x4 definition. This newline is there in all other files, but apparently not here.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 946042;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

Here, the variable int32x4 is actually used, so it shouldn't be removed. `var float32x4 = SIMD.float32x4;` can be removed, as this variable is not used anywhere; only `SIMD.float32x4` is used.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromint32x4bits.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 946042;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

Same as for float32x4fromint32x4.js.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4getters.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 938728;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var float32x4 = SIMD.float32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4handle.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 938728;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var float32x4 = SIMD.float32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4lessthanorequal.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 946042;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

nit: this variable declaration has been replaced by an empty line. Please remove this line.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4mul.js
@@ -5,2 @@
>  
>  var summary = 'float32x4 add';

The summary has not been changed here. Please change it into 'float32x4 mul'. Changing the incorrect summaries is also a part of the bug.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4or.js
@@ -5,2 @@
>  
>  var summary = 'float32x4 and';

Please change the summary into 'float32x4 or'.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4reify.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 938728;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var float32x4 = SIMD.float32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4setter.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 938728;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var float32x4 = SIMD.float32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4xor.js
@@ -5,2 @@
>  
>  var summary = 'float32x4 and';

Please change the summary into 'float32x4 xor'.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4alignment.js
@@ -4,1 @@
>  var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var int32x4 = SIMD.int32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4.js
@@ -1,3 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 946042;
> -var float32x4 = SIMD.float32x4;

Here, the variable float32x4 is actually used, so it shouldn't be removed. `var int32x4 = SIMD.int32x4;` can be removed here, as this variable is not used; only `SIMD.int32x4` is used.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4handle.js
@@ -4,1 @@
>  var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var int32x4 = SIMD.int32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4reify.js
@@ -4,1 @@
>  var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var int32x4 = SIMD.int32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4setter.js
@@ -4,1 @@
>  var int32x4 = SIMD.int32x4;

While you are here, please add a newline between the `var int32x4 = SIMD.int32x4;` and the summary. This newline has not been added here yet.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4withflag.js
@@ -4,3 @@
>  var int32x4 = SIMD.int32x4;
>  
>  var summary = 'int32x4 with';

The summary has not been changed here. It should be 'int32x4 withFlag'. Changing the incorrect summaries is also a part of the bug.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8490662 [details] [diff] [review]
Bug 1063946.patch

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

Thanks for working on this! See ProgramFOX's comments (feel free to ignore adding the new lines and such, it really doesn't matter in test files).
Once you're done, please make sure to compile a JS shell [0], to run the tests locally [1] and to ensure that they run fine. If you have any question, feel free to come over on IRC (#jsapi or #ionmonkey on irc.mozilla.org) or to comment here, and somebody will be glad to help you :)

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Test

::: js/src/tests/ecma_6/TypedObject/simd/float32x4setter.js
@@ -5,1 @@
>  var summary = 'float32x4 setting';

please change this summary to float32x4 setters

::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4.js
@@ -4,1 @@
>  var int32x4 = SIMD.int32x4;

Can you also (in this file and any other test file) make sure that we use the int32x4 variable, rather than having the long form SIMD.int32x4 here?

::: js/src/tests/ecma_6/TypedObject/simd/int32x4setter.js
@@ -4,2 @@
>  var int32x4 = SIMD.int32x4;
>  var summary = 'int32x4 setting';

Can you change this summary to int32x4 setters, please?

Comment 12

[Aman Singh]

(In reply to Benjamin Bouvier [:bbouvier] from comment #11)

> ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat32x4.js
> @@ -4,1 @@
> >  var int32x4 = SIMD.int32x4;
> 
> Can you also (in this file and any other test file) make sure that we use
> the int32x4 variable, rather than having the long form SIMD.int32x4 here?
> 
> ::: 

Benjamin what you want me to do in thethis comment?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Aman Singh from comment #12)
> Benjamin what you want me to do in thethis comment?

At the top of the test file, we have this renaming:

var int32x4 = SIMD.int32x4; // (1)

And later in the file, we're using this:

var c = SIMD.int32x4.fromFloat32x4(a);

So, that would be nicer to make use of the renaming (1) here:

var c = int32x4.fromFloat32x4(a);

And extend this to all files, making sure that we use the renamed versions rather than having the long form SIMD.type in the test() functions. Is it clearer now ? :)

Comment 14

[Aman Singh]

Yes. Thanks :)

Comment 15

[Aman Singh]

Attachment: BUG 1063946.patch

Please review this. I have made all the suggested changes.

Comment 16

[Aman Singh]

Attachment: final bug 1063946.patch

Please review it.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8491617 [details] [diff] [review]
final bug 1063946.patch

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

See comment for float32x4select. Please make sure the tests still pass once you've applied your modifications.

::: js/src/tests/ecma_6/TypedObject/simd/bug953270.js
@@ -15,2 @@
>  var a = int32x4((4294967295), 200, 300, 400);
>  var c = SIMD.float32x4.fromInt32x4Bits(a);

This could get some shortening love as well:

var c = float32x4.fromtInt32x4Bits(a);

::: js/src/tests/ecma_6/TypedObject/simd/float32x4select.js
@@ -1,4 @@
>  // |reftest| skip-if(!this.hasOwnProperty("SIMD"))
>  var BUGNUMBER = 1060437;
>  var float32x4 = SIMD.float32x4;
> -var int32x4 = SIMD.int32x4;

This test won't pass as int32x4 are used below. Are you sure you've compiled the shell and ran the tests as indicated in my previous comment?

Comment 18

[Aman Singh]

Attachment: Bug1063946.patch

I have build mozilla central after making all the changes and it was running fine. Do I need to specifically build Js shell or its fine?

Comment 19

[Aman Singh]

Attachment: Bug1063946.patch

I have build mozilla central after making all the changes and it was running fine. Do I need to specifically build Js shell or its fine?

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8493289 [details] [diff] [review]
Bug1063946.patch

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

Having it build isn't enough. You need to make sure that tests pass [0] [1].

Here is the list of tests that break with your patch:

➜  wd64  ../../inbound/js/src/tests/jstests.py ./dist/bin/js simd
[14| 0| 0| 0]  21% ==========>                                        |   0.6s
REGRESSION - ecma_6/TypedObject/simd/int32x4select.js
[48| 1| 0| 0]  76% ======================================>            |   1.9s
REGRESSION - ecma_6/TypedObject/simd/int32x4getters.js
[59| 2| 0| 0]  95% ===============================================>   |   2.2s
REGRESSION - ecma_6/TypedObject/simd/bug953270.js
[59| 3| 0| 0]  96% ================================================>  |   2.3s
REGRESSION - ecma_6/TypedObject/simd/int32x4fromfloat32x4.js
[60| 4| 0| 0] 100% ==================================================>|   2.3s
REGRESSIONS
    ecma_6/TypedObject/simd/int32x4select.js
    ecma_6/TypedObject/simd/int32x4getters.js
    ecma_6/TypedObject/simd/bug953270.js
    ecma_6/TypedObject/simd/int32x4fromfloat32x4.js
FAIL

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Test

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Of course, feel free to show up in #jsapi and ask us for more details about how to compile or run tests, anybody will be glad to help :) (my irc handle is bbouvier)

Comment 22

[Aman Singh]

Attachment: upload.patch

Please review it.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8493849 [details] [diff] [review]
upload.patch

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

[15| 0| 0| 0]  23% ==========>                                        |   0.7s
REGRESSION - ecma_6/TypedObject/simd/int32x4select.js
[63| 1| 0| 0] 100% ==================================================>|   2.3s
REGRESSIONS
    ecma_6/TypedObject/simd/int32x4select.js
FAIL

Sorry, but you should definitely check that tests pass before submitting a patch. If I didn't provide enough help on IRC, feel free to reach me again :)
For what it's worth, testing only the simd test cases can be done with:
➜  ./jstests.py ./path/to/js simd

::: js/src/tests/ecma_6/TypedObject/simd/int32x4select.js
@@ -19,5 @@
>    assertEq(c.z, 3);
>    assertEq(c.w, 4);
>  
> -  var a2 = int32x4(INT32_MAX,INT32_MIN,9,16)
> -  var c = SIMD.int32x4.select(sel_ttff,a2,b);

Please don't delete any existing tests, in any test case.

Comment 24

[:ProgramFOX]

Do you plan to continue working on this bug, Aman?

Comment 25

[Iskandar Rafikov]

Can I continue work on this bug?

Comment 26

[:ProgramFOX]

Hi Iskandar,

Yes, you can. The person who wanted to take it originally hasn't replied for more than a month, so you can safely assume that they won't work on it further.

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Iskandar Rafikov from comment #25)
> Can I continue work on this bug?

Iskandar: Please go ahead if you'd like too. However, I see that you asked this for quite a few bugs recently, so please focus on one single bug, to start. Thanks!

(In reply to ProgramFOX from comment #26)
Hey ProgramFOX, while I appreciate you opening and answering in this bug, I think it is a bit early to think about mentoring. Thanks!

All:
The original contributor has been inactive for a month despite a needinfo, so this bug is open.
I'd rather like to morph this bug into simplifying SIMD.js tests. Having a single test file by type/operation couple seems pretty inefficient, so here is what I propose for a start: have all arithmetic (add, sub, div, mul) tests in a single test file, and delete the equivalent type/operation files. I'd advise people to look at tests/ecma_6/TypedObjects/simd/{comparisons.js, shell.js} to understand how to make these tests simpler without having to type a lot of code.

Comment 28

[Iskandar Rafikov]

Yes,  I asked for a few bugs, but now I'm working on Bug 1101553. When I finish work , i'll continue work on this bug, if it will be possible.

Comment 29

[Jan-Christoph Klie]

Hello, 
I am interested to work on this issue, but after I checked out the most recent trunk, I do not find the files you were talking about (js/src/tests/ecma_6/TypedObject/simd). What I found was js/src/tests/ecma_7/SIMD, which contains many test files for every SIMD instruction. Is it still an open task to refactor it in a way which groups these tests not by type/operation but by semantics (add, sub, div, mul, ...)?

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan-Christoph Klie from comment #29)
> Hello, 
> I am interested to work on this issue, but after I checked out the most
> recent trunk, I do not find the files you were talking about
> (js/src/tests/ecma_6/TypedObject/simd). What I found was
> js/src/tests/ecma_7/SIMD, which contains many test files for every SIMD
> instruction. Is it still an open task to refactor it in a way which groups
> these tests not by type/operation but by semantics (add, sub, div, mul, ...)?

Hi, thanks for your interest! This bug is still valid, indeed, but I'd wait for the float64x2 bug to get fully landed before working on this one, as it will introduce a few changes and is really close to land. Marking bug 1031203 as blocking this one.

Comment 31

[Benjamin Bouvier [:bbouvier] (inactive)]

Hey Jan-Christoph, are you still interested in working on this bug?

If so, I think it'd make sense to start by grouping / generalizing unary arithmetic operations, in a single file (that means merging abs, neg, reciprocal, reciprocalSqrt). The file could be named "unary-arithmetic.js", for instance.

Comment 32

[Jan-Christoph Klie]

Hello Benjamin,

I am still interested in this bug, but I still have the problem that I do not find the files specified at the beginning of this bug report. The only place I see SIMD tests is in ecma_7. There isn't even a folder /js/src/tests/ecma_6/TypedObject in my repository. Can you please point me to where the files we speak of are?

Comment 33

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for sticking with this bug, and really sorry for not having answered your question before.  Indeed, the test files are now located in tests/ecma_7/SIMD, as SIMD won't be shipped with es6 / es2015 but much later probably.  For what it's worth, TypedObjects also have moved, as they won't be shipped in es6 as well.

Comment 34

[Benjamin Bouvier [:bbouvier] (inactive)]

So this is blocking bug 1124291, which otherwise would add a lot of new test files, which we'd like to avoid, if that's possible.  As a matter of fact, as it's been a week that Jan-Christoph hasn't answered, I am fine with anybody else taking it.  Jan-Christoph, feel free to post a message here saying you're working on it, if that's the case, but please do it rather quickly :)

Comment 35

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping.patch

Sine nobody is working on this. I could do it. My proposal would be to split it into:
- unary-operations
- binary-operations
- conversions
- 4th type for alignment, getters, handle, setter, reify

perhaps split all of them into float32x4-(category) and int32x4-(category).

I've attached an example how float-32x4-unary-operations.js would look like.

Comment 36

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8574386 [details] [diff] [review]
Bug_1063946_grouping.patch

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

Looks like a great start, thanks!

(In reply to flomerz from comment #35)
> Created attachment 8574386 [details] [diff] [review]
> Bug_1063946_grouping.patch
> 
> Sine nobody is working on this. I could do it. My proposal would be to split
> it into:
> - unary-operations
> - binary-operations
> - conversions
> - 4th type for alignment, getters, handle, setter, reify
Looks good. The last type could be called "details" or "typedobjects", as these refer to implementation details (SIMD objects are typed objects and should behave as such).

> 
> perhaps split all of them into float32x4-(category) and int32x4-(category).
For operations shared among all types (float32x4, int32x4), I'd like to have them in a single file, even.  Of course, operations specific to a type (e.g. float32x4.sqrt is specific to float32x4) can be kept in a float32x4-unary.js file.

> 
> I've attached an example how float-32x4-unary-operations.js would look like.
Looks good, keep it up! For what it's worth, you can ask feedback from myself next time, by entering :bbouvier (note the ":") in the feedback? field, and then I'll get a notification.  So it is a better needinfo :)

::: js/src/tests/ecma_7/SIMD/float32x4-unaray-operations.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))

nit: filename contains unaray => unary, please :)

@@ +1,4 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var float32x4 = SIMD.float32x4;
> +
> +function simdToArray(v) {

i don't think you need them, as they're present in shell.js, which is automatically loaded

@@ +10,5 @@
> +  else
> +    throw new TypeError("Unknown SIMD kind.");
> +}
> +
> +function testUnaryFunc(v, simdFunc, func) {

ditto

@@ +42,5 @@
> +
> +  testUnaryFunc(v, SIMD.float32x4.not, notf);
> +}
> +
> +function testFloat32x4reciprocal(v) {

Mostly curious here, but aren't there any issues when testing reciprocal and reciprocalSqrt? As these are supposed to be approximations, I'd expect some test failures... (in particular when testing with --tbpl)

Comment 37

[Florian Dieminger [:fiji]]

Thanks for the feedback Benjamin. I'll give you an update how far I've come in the next couple of days.

You're right, reciprocal and reciprocalSqrt tests dont pass when testing with --tbpl. I'm gonna fix that.

Comment 38

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping.patch

Hey Benjamin,

a small update.

I decided to ditch the float32x4-unary-operations and put them into unary-opeations.js.

unary and binary operations should be almost done. This time I checked with --tbpl, too. I tried to keep the tests as close to the original ones as possible. Sometimes, the expected values could be easily computed.

I'll preoceed and start on "conversions" and "typedobjects".

Comment 39

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8576806 [details] [diff] [review]
Bug_1063946_grouping.patch

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

Looks really good! That's a nice patch. Before keeping on doing the same thing for other operations, I'd like you to fix the alignment in arrays, which is disturbing to read. Could you do the remaining work in other patches, so that I don't need to re-read the entire patch? That would speed up giving you feedback and review (you can do this with the record / crecord extensions). Ideally, I'd like to have this patch split up in two parts (unary vs binary), so that it makes reviewing even easier, but that's fine if it stays as is :)

::: js/src/tests/ecma_7/SIMD/binary-operations.js
@@ +8,5 @@
> +    return Math.fround(Math.fround(a) + Math.fround(b));
> +  }
> +
> +  var float32x4vals = [
> +    [float32x4(1, 2, 3, 4)

nit: here and everywhere, can you align the lines at the commas or the values, so that the arrays don't look too crazy, please?
For instance like this:

[
  [float32x4(1,2,3,4), float32x4(10, 20, 30, 40)],
  [...]
];

::: js/src/tests/ecma_7/SIMD/unary-operations.js
@@ +44,5 @@
> +}
> +
> +function testFloat32x4reciprocalSqrt() {
> +  function reciprocalsqrtf(a) {
> +    return Math.fround(Math.sqrt(1 / Math.fround(a)));

This is actually sqrt first then 1/x, but that would mean changing the C++ as well, so that can be another patch / bug.

Comment 40

[Florian Dieminger [:fiji]]

Thanks for the input Benjamin, sorry for the messed up array layout, I'll fix that.

I will continue. It may take a few days until I'm finished. I'm on vacation right now but I'll find some time to do it.

Comment 41

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to flomerz from comment #40)
> Thanks for the input Benjamin, sorry for the messed up array layout, I'll
> fix that.
> 
> I will continue. It may take a few days until I'm finished. I'm on vacation
> right now but I'll find some time to do it.

Sure thing! Take your time, there is no hurry :)

Comment 42

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_binary.patch

This is the first part of the patch set. I've split the patch into the four groups. Please let me know if they need some improvment.

- binary operations -

Comment 43

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_conversions.patch

Comment 44

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_typedobjects.patch

Comment 45

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_unary.patch

Comment 46

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8581607 [details] [diff] [review]
Bug_1063946_grouping_binary.patch

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

Nice, thanks! Setting r+ as it looks good enough. I've put a few comments though, just in case you'd be willing to make your patch look perfect :)

Maybe we could even share the int32x4 and float32x4 value arrays in some ways, across functions (that is, share the float32x4 values across the float32x4 test functions), but that can be done in another patch / bug / it isn't really high priority.

::: js/src/tests/ecma_7/SIMD/binary-operations.js
@@ +14,5 @@
> +    [[NaN, -0, Infinity, -Infinity], [0, -0, -Infinity, -Infinity]]
> +  ];
> +
> +  for (var [v,w] of vals) {
> +    testBinaryFunc(float32x4(...v), float32x4(...w), float32x4.add, addf);

hehe, nice

@@ +149,5 @@
> +
> +function testInt32x4and() {
> +  var valsExp = [
> +    [[1, 2, 3, 4], [10, 20, 30, 40], [0, 0, 2, 0]],
> +    [[INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN], [INT32_MIN, INT32_MAX, INT32_MAX, INT32_MIN], [(INT32_MAX & INT32_MIN) | 0, (INT32_MIN & INT32_MAX) | 0, (INT32_MAX & INT32_MAX) | 0, (INT32_MIN & INT32_MIN) | 0]]

feel free to create a local 'andi' function, as for float32x4, and use it with testBinaryFunc:

function andi(x, y) { return (x & y) | 0; }

It will look better than putting results like this here.

@@ +160,5 @@
> +
> +function testInt32x4mul() {
> +  var valsExp = [
> +    [[1, 2, 3, 4], [10, 20, 30, 40], [10, 40, 90, 160]],
> +    [[INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN], [-1, -1, INT32_MIN, INT32_MIN], [(INT32_MAX * -1) | 0, (INT32_MIN * -1) | 0, (INT32_MAX * INT32_MIN) | 0, (INT32_MIN * INT32_MIN) | 0]]

ditto

@@ +171,5 @@
> +
> +function testInt32x4or() {
> +  var valsExp = [
> +    [[1, 2, 3, 4], [10, 20, 30, 40], [11, 22, 31, 44]],
> +    [[INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN], [INT32_MIN, INT32_MAX, INT32_MAX, INT32_MIN], [(INT32_MAX | INT32_MIN) | 0, (INT32_MIN | INT32_MAX) | 0, (INT32_MAX | INT32_MAX) | 0, (INT32_MIN | INT32_MIN) | 0]]

ditto

@@ +182,5 @@
> +
> +function testInt32x4sub() {
> +  var valsExp = [
> +    [[10, 20, 30, 40], [1, 2, 3, 4], [9, 18, 27, 36]],
> +    [[INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN], [-1, 1, INT32_MAX, INT32_MIN], [(INT32_MAX - -1) | 0, (INT32_MIN - 1) | 0, 0, 0]]

ditto (or inline all results, not only a part of them :))

@@ +193,5 @@
> +
> +function testInt32x4xor() {
> +  var valsExp = [
> +    [[1, 2, 3, 4], [10, 20, 30, 40], [11, 22, 29, 44]],
> +    [[INT32_MAX, INT32_MIN, INT32_MAX, INT32_MIN], [INT32_MIN, INT32_MAX, INT32_MAX, INT32_MIN], [(INT32_MAX ^ INT32_MIN) | 0, (INT32_MIN ^ INT32_MAX) | 0, (INT32_MAX ^ INT32_MAX) | 0, (INT32_MIN ^ INT32_MIN) | 0]]

ditto

Comment 47

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8581609 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch

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

I like it!

::: js/src/tests/ecma_7/SIMD/conversions.js
@@ +4,5 @@
> +var int32x4 = SIMD.int32x4;
> +
> +function testFloat32x4FromFloat64x2() {
> +  function expected(v) {
> +    return [...(v.map(x => Math.fround(x))), 0, 0];

v.map(Math.fround)

@@ +34,5 @@
> +}
> +
> +function testFloat32x4FromInt32x4() {
> +  function expected(v) {
> +    return v.map(x => Math.fround(x));

v.map(Math.fround)

@@ +59,5 @@
> +}
> +
> +function testFloat64x2FromFloat32x4() {
> +  function expected(v) {
> +    return v.slice(0, 2).map(x => Math.fround(x));

ditto

Comment 48

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8581611 [details] [diff] [review]
Bug_1063946_grouping_unary.patch

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

Cool!

::: js/src/tests/ecma_7/SIMD/unary-operations.js
@@ +4,5 @@
> +
> +
> +function testFloat32x4abs() {
> +  function absf(a) {
> +    return Math.fround(Math.abs(a));

I'd prefer composition to happen the other way around (even if it's strictly equal :))

return Math.abs(Math.fround(a))

@@ +19,5 @@
> +}
> +
> +function testFloat32x4neg() {
> +  function negf(a) {
> +    return Math.fround(a * -1);

Same here :)

Comment 49

[Benjamin Bouvier [:bbouvier] (inactive)]

Nice job! When you feel your patches are ready, please submit them again for review, so that I make a more careful review, but I think these are all mostly perfect and ready to land :)

Comment 50

[Florian Dieminger [:fiji]]

I've polished it a little. But I'll have to wait for a better internet connection to upload the files. I'm in the middle of nowhere in New Mexico. Maybe in one or two days.

Comment 51

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_binary.patch

Should be fine. I did not have much additional time, but I can live with this state.

Comment 52

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_conversions.patch

Comment 53

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_typedobjects.patch

Comment 54

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_unary.patch

Comment 55

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8584645 [details] [diff] [review]
Bug_1063946_grouping_binary.patch

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

Looks good to me

Comment 56

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #48)
> Comment on attachment 8581611 [details] [diff] [review]
> Bug_1063946_grouping_unary.patch
> 
> Review of attachment 8581611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool!
> 
> ::: js/src/tests/ecma_7/SIMD/unary-operations.js
> @@ +4,5 @@
> > +
> > +
> > +function testFloat32x4abs() {
> > +  function absf(a) {
> > +    return Math.fround(Math.abs(a));
> 
> I'd prefer composition to happen the other way around (even if it's strictly
> equal :))
> 
> return Math.abs(Math.fround(a))
> 
> @@ +19,5 @@
> > +}
> > +
> > +function testFloat32x4neg() {
> > +  function negf(a) {
> > +    return Math.fround(a * -1);
> 
> Same here :)

Can you please upload a new patch with these comments addressed, and r? me on it?

Comment 57

[Benjamin Bouvier [:bbouvier] (inactive)]

Oops, nevermind that comment, didn't see that the former patch wasn't marked as obsolete, my bad.

Comment 58

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8584649 [details] [diff] [review]
Bug_1063946_grouping_unary.patch

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

Nice, thank you! I've set r+ despite the comment, so that you can upload a new version of this patch with the r+ that you can set yourselves (no need to re-ask for review).

::: js/src/tests/ecma_7/SIMD/unary-operations.js
@@ +50,5 @@
> +}
> +
> +function testFloat32x4reciprocalSqrtApproximation() {
> +  function reciprocalsqrtf(a) {
> +    return Math.fround(Math.sqrt(1 / Math.fround(a)));

See bug 1150836, composition order has changed inside the parenthesis.

Comment 59

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8584646 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch

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

Cool.

Comment 60

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8584648 [details] [diff] [review]
Bug_1063946_grouping_typedobjects.patch

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

Nice, thanks for replacing the dynamic getprop accesses by static ones.

Sorry for the long time taken to review, last week was PTO. Thank you for the contribution. Now that all patches are r+'d, minor comments (from previous reviews) need to be addressed and patches need to be rebased. Then we'll need to run them in our testing infrastructure, through the try server. If you already have the try commit access, feel free to do so by yourself, otherwise I'll do it on your behalf. Then, these changes will get checked in! :)

Comment 61

[Florian Dieminger [:fiji]]

I've tried to rebase the patches and change add the last change. 

Since, bug 1150836 has not yet checked in my fixes would currently break the tests. I'll wait for that patch and rebase then.

And no, I do not have try commit access yet (but would like to).

Comment 62

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_unary.patch

Address latest comments and rebase.

Comment 63

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_binary.patch

Comment 64

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_conversions.patch

Comment 65

[Florian Dieminger [:fiji]]

Attachment: Bug_1063946_grouping_typedobjects.patch

Comment 66

[Florian Dieminger [:fiji]]

I've rebased the patches and addressed bug 1150836. As I mentioned I do not have try commit access yet, feel free to give that to me :).

Comment 67

[Benjamin Bouvier [:bbouvier] (inactive)]

As this is your first bug, I've pushed your patches to try on your behalf. For your four patches, I think we'll need to add "r=bbouvier" at the end, as recommended in [0]. As I'll push your patches to make sure they get checked in quickly, I'll do it this time :) I'll push your changes as soon as the try run returns green results, indicating everything went well (local testing says things should be working fine, but we're never far from a bad surprise). 

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a1162305f9f

Comment 68

[Benjamin Bouvier [:bbouvier] (inactive)]

The red failure in the try build in comment 67 was an intermittent issue, so everything should be fine here. Here's your patch checked in in inbound, where it will start riding the trains:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d41fe4090e52
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/56fd246f5371
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/404b3ca74d7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5018b45f1bc0

Thanks for your patches! Would you be interested in working on another bug?

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d41fe4090e52
https://hg.mozilla.org/mozilla-central/rev/56fd246f5371
https://hg.mozilla.org/mozilla-central/rev/404b3ca74d7e
https://hg.mozilla.org/mozilla-central/rev/5018b45f1bc0

Comment 70

[Florian Dieminger [:fiji]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #68)
> Thanks for your patches! Would you be interested in working on another bug?

Sure. If you could point me to a "good next" bug, that would be great. I'm comfortable with C++, too.

Comment 71

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Florian Merz from comment #70)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #68)
> > Thanks for your patches! Would you be interested in working on another bug?
> 
> Sure. If you could point me to a "good next" bug, that would be great. I'm
> comfortable with C++, too.

Cool! Feel free to give a look at bug 1155211 ;)