SIMD (interpreter): Group tests in logical units

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ProgramFOX, Assigned: Florian Merz, Mentored)

Tracking

unspecified
mozilla40
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(4 attachments, 16 obsolete attachments)

12.83 KB, patch
Florian Merz
: review+
Details | Diff | Splinter Review
22.64 KB, patch
Florian Merz
: review+
Details | Diff | Splinter Review
19.14 KB, patch
Florian Merz
: review+
Details | Diff | Splinter Review
31.91 KB, patch
Florian Merz
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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'
(Reporter)

Comment 1

4 years ago
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.
Depends on: 948379
(Reporter)

Comment 2

4 years ago
In comment 1, I meant "when that patch is landed" instead of "after that bug is fixed".
(Reporter)

Updated

4 years ago
Whiteboard: [good first bug][lang=js]
Thanks for filing this bug, ProgramFOX!
Mentor: benj@benj.me

Comment 4

4 years ago
I want to work on this bug. Please help me.
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 6

4 years ago
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.
No longer depends on: 948379
(Reporter)

Comment 7

4 years ago
Hey Aman,

Any news about this? Are you making progress on the bug?
Flags: needinfo?(ug201310004)

Comment 8

4 years ago
Yes I was busy with my exams but I have started working on this.
Flags: needinfo?(ug201310004)

Comment 9

4 years ago
Created attachment 8490662 [details] [diff] [review]
Bug 1063946.patch

Please review the Patch.
Attachment #8490662 - Flags: review?(benj)
(Reporter)

Comment 10

4 years ago
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 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?
Attachment #8490662 - Flags: review?(benj) → feedback+

Comment 12

4 years ago
(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?
(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

4 years ago
Yes. Thanks :)

Comment 15

4 years ago
Created attachment 8491494 [details] [diff] [review]
BUG 1063946.patch

Please review this. I have made all the suggested changes.
Attachment #8491494 - Flags: review?(benj)

Comment 16

4 years ago
Created attachment 8491617 [details] [diff] [review]
final bug 1063946.patch

Please review it.
Attachment #8491494 - Attachment is obsolete: true
Attachment #8491494 - Flags: review?(benj)
Attachment #8491617 - Flags: review?(benj)
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?
Attachment #8491617 - Flags: review?(benj)

Comment 18

4 years ago
Created attachment 8493287 [details] [diff] [review]
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?
Attachment #8491617 - Attachment is obsolete: true
Attachment #8493287 - Flags: review?(benj)

Comment 19

4 years ago
Created attachment 8493289 [details] [diff] [review]
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?
Attachment #8493287 - Attachment is obsolete: true
Attachment #8493287 - Flags: review?(benj)
Attachment #8493289 - Flags: review?(benj)
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
Attachment #8493289 - Flags: review?(benj)
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

4 years ago
Created attachment 8493849 [details] [diff] [review]
upload.patch

Please review it.
Attachment #8493289 - Attachment is obsolete: true
Attachment #8493849 - Flags: review?(benj)
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.
Attachment #8493849 - Flags: review?(benj) → review-
(Reporter)

Comment 24

3 years ago
Do you plan to continue working on this bug, Aman?
Flags: needinfo?(ug201310004)

Comment 25

3 years ago
Can I continue work on this bug?
(Reporter)

Comment 26

3 years ago
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.
(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.
Flags: needinfo?(ug201310004)
Summary: SIMD test cases: unused variables and sometimes incorrect summaries → SIMD (interpreter): Group tests in logical units

Comment 28

3 years ago
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

3 years ago
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, ...)?
(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.
Depends on: 1031203
Flags: needinfo?(benj)
Flags: needinfo?(benj)
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.
Flags: needinfo?(git)

Comment 32

3 years ago
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?
Flags: needinfo?(git)
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.
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 :)
Blocks: 1124291
(Assignee)

Comment 35

3 years ago
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

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.
Flags: needinfo?(benj)
Attachment #8574386 - Flags: feedback?
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)
Attachment #8574386 - Flags: feedback? → feedback+
Flags: needinfo?(benj)
(Assignee)

Comment 37

3 years ago
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.
(Assignee)

Comment 38

3 years ago
Created attachment 8576806 [details] [diff] [review]
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".
Attachment #8574386 - Attachment is obsolete: true
Attachment #8576806 - Flags: feedback?(benj)
Attachment #8490662 - Attachment is obsolete: true
Attachment #8493849 - Attachment is obsolete: true
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.
Attachment #8576806 - Flags: feedback?(benj) → feedback+
(Assignee)

Comment 40

3 years ago
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.
(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 :)
(Assignee)

Comment 42

3 years ago
Created attachment 8581607 [details] [diff] [review]
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 -
Attachment #8576806 - Attachment is obsolete: true
Attachment #8581607 - Flags: feedback?(benj)
(Assignee)

Comment 43

3 years ago
Created attachment 8581609 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch
(Assignee)

Comment 44

3 years ago
Created attachment 8581610 [details] [diff] [review]
Bug_1063946_grouping_typedobjects.patch
(Assignee)

Comment 45

3 years ago
Created attachment 8581611 [details] [diff] [review]
Bug_1063946_grouping_unary.patch
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
Attachment #8581607 - Flags: feedback?(benj) → review+
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
Attachment #8581609 - Flags: feedback+
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 :)
Attachment #8581611 - Flags: feedback+
Attachment #8581610 - Flags: feedback+
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 :)
Assignee: nobody → flomerz
Status: NEW → ASSIGNED
(Assignee)

Comment 50

3 years ago
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.
(Assignee)

Comment 51

3 years ago
Created attachment 8584645 [details] [diff] [review]
Bug_1063946_grouping_binary.patch

Should be fine. I did not have much additional time, but I can live with this state.
Attachment #8581607 - Attachment is obsolete: true
Attachment #8584645 - Flags: review?(benj)
(Assignee)

Comment 52

3 years ago
Created attachment 8584646 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch
Attachment #8584646 - Flags: review?(benj)
(Assignee)

Comment 53

3 years ago
Created attachment 8584648 [details] [diff] [review]
Bug_1063946_grouping_typedobjects.patch
Attachment #8581609 - Attachment is obsolete: true
Attachment #8581610 - Attachment is obsolete: true
Attachment #8584648 - Flags: review?(benj)
(Assignee)

Comment 54

3 years ago
Created attachment 8584649 [details] [diff] [review]
Bug_1063946_grouping_unary.patch
Attachment #8584649 - Flags: review?(benj)
Comment on attachment 8584645 [details] [diff] [review]
Bug_1063946_grouping_binary.patch

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

Looks good to me
Attachment #8584645 - Flags: review?(benj) → review+
(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?
Flags: needinfo?(flomerz)
Oops, nevermind that comment, didn't see that the former patch wasn't marked as obsolete, my bad.
Flags: needinfo?(flomerz)
Attachment #8581611 - Attachment is obsolete: true
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.
Attachment #8584649 - Flags: review?(benj) → review+
Comment on attachment 8584646 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch

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

Cool.
Attachment #8584646 - Flags: review?(benj) → review+
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! :)
Attachment #8584648 - Flags: review?(benj) → review+
(Assignee)

Comment 61

3 years ago
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).
(Assignee)

Comment 62

3 years ago
Created attachment 8591852 [details] [diff] [review]
Bug_1063946_grouping_unary.patch

Address latest comments and rebase.
Attachment #8584649 - Attachment is obsolete: true
Attachment #8591852 - Flags: review+
(Assignee)

Comment 63

3 years ago
Created attachment 8591853 [details] [diff] [review]
Bug_1063946_grouping_binary.patch
Attachment #8584645 - Attachment is obsolete: true
Attachment #8591853 - Flags: review+
(Assignee)

Comment 64

3 years ago
Created attachment 8591855 [details] [diff] [review]
Bug_1063946_grouping_conversions.patch
Attachment #8584646 - Attachment is obsolete: true
Attachment #8591855 - Flags: review+
(Assignee)

Comment 65

3 years ago
Created attachment 8591856 [details] [diff] [review]
Bug_1063946_grouping_typedobjects.patch
Attachment #8584648 - Attachment is obsolete: true
Attachment #8591856 - Flags: review+
(Assignee)

Comment 66

3 years ago
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 :).
Flags: needinfo?(benj)
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
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?
Flags: needinfo?(benj)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 70

3 years ago
(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.
(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 ;)
You need to log in before you can comment on or make changes to this bug.