1050214 - SIMD: add shuffle's masks constants to the SIMD object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:ProgramFOX]

The SIMD straw man proposal contains some SIMD properties [1] that can be used as parameter to the shuffle and shuffleMix method so you don't have to hardcode the number. These properties are not yet implemented in the SIMD module in the core, so these properties and a test case for this should be added.

[1]: https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js (starting at line 1190)

Comment 1

[:ProgramFOX]

Forgot that GitHub has a feature when posting the link in my report, here is a new link with the lines highlighted:

https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd.js#L1190-1450

Comment 2

[:ProgramFOX]

"has a feature when posting" had to be "has a line-highlighting feature".

Comment 3

[:ProgramFOX]

Pushed to try my current patch because I got an error in a file that I didn't make changes in:

https://tbpl.mozilla.org/?tree=Try&rev=68b86b3ec4b5

Comment 4

[:ProgramFOX]

Because the build logs said:
>+js/src/builtin/SIMD.cpp:15:17: error:
>+    "builtin/SIMDShuffleMaskConstants.h" should be included after "jsapi.h"
I changed this and created a new try push:

https://tbpl.mozilla.org/?tree=Try&rev=b2f578af2dc5

Comment 5

[:ProgramFOX]

Because I got a test fail like the above again, I cancelled the build and created a new one.

https://tbpl.mozilla.org/?tree=Try&rev=694f254fbc4c

Comment 6

[:ProgramFOX]

Attachment: Added shuffle's mask constants to the SIMD object

This patch builds successfully and the tests run without errors:
https://tbpl.mozilla.org/?tree=Try&rev=c2d3294c4715
But it gives a SpiderMonkey Fail-On-Warnings Build fail:
>'operator new(unsigned' present at SIMD.cpp:358
I'm not sure why I get this, but because the build runs fine, I think it is OK.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8475085 [details] [diff] [review]
Added shuffle's mask constants to the SIMD object

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

Sorry, it's really not fine to have a build failure like this, even if it's a build warning :/
It seems you're on the right track though.

::: js/src/builtin/SIMDShuffleMaskConstants.h
@@ +6,5 @@
> +
> +#ifndef builtin_SIMDShuffleMaskConstants_h
> +#define builtin_SIMDShuffleMaskConstants_h
> +
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)

could you avoid to have the maskStr parameter, as it is just "##mask##" ?

@@ +7,5 @@
> +#ifndef builtin_SIMDShuffleMaskConstants_h
> +#define builtin_SIMDShuffleMaskConstants_h
> +
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +    

nit: trailing whitespace

@@ +10,5 @@
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +    
> +
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \
> +    JS::Value *v##mask = new JS::Value();                   \

Here's the reason for your build failure: you shouldn't create a jsvalue like this. Instead, please use the constructor RootedValue(cx, Int32Value(42));

@@ +12,5 @@
> +
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \
> +    JS::Value *v##mask = new JS::Value();                   \
> +    v##mask->setInt32(value);                               \
> +    RootedValue rv##mask(cx, *v##mask);                     \

nit: rename this to mask##mask

@@ +13,5 @@
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \
> +    JS::Value *v##mask = new JS::Value();                   \
> +    v##mask->setInt32(value);                               \
> +    RootedValue rv##mask(cx, *v##mask);                     \
> +    JSObject::defineProperty(cx, SIMD, cx->names().mask,    \

That looks wrong. You're re-defining cx->names().mask here. Shouldn't your tests be failing?

@@ +18,5 @@
> +                     rv##mask, nullptr, nullptr,            \
> +                     JSPROP_READONLY | JSPROP_PERMANENT);
> +
> +
> +#define FOR_EACH_SIMD_SHUFFLE_MASK(_macro, macro)                \

Can you use better variable names? It's very unclear what's the difference between the twos are.

@@ +275,5 @@
> +    _macro(macro, WWWX, "WWWX", 0x3F)                            \
> +    _macro(macro, WWWY, "WWWY", 0x7F)                            \
> +    _macro(macro, WWWZ, "WWWZ", 0xBF)                            \
> +    _macro(macro, WWWW, "WWWW", 0xFF)                            \
> +    _macro(macro, XX, "XX", 0x0)                               \

nit: could you have all of these aligned please?

::: js/src/tests/ecma_6/TypedObject/simd/shufflemaskconstants.js
@@ +1,4 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 1050214;
> +
> +var summary = "SIMD shuffle mask's constants";

Nice test.

Comment 8

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> Comment on attachment 8475085 [details] [diff] [review]
> Added shuffle's mask constants to the SIMD object
> 
> Review of attachment 8475085 [details] [diff] [review]:
> -----------------------------------------------------------------
> That looks wrong. You're re-defining cx->names().mask here. Shouldn't your
> tests be failing?
They run without problems.

I'm currently fixing this bug, and changing the RootedValue constructor worked fine, but replacing maskStr with "##mask##" does not. It builds, but the tests fail, so I pushed to try to see whether I would get a warning:

https://tbpl.mozilla.org/?tree=Try&rev=c5bbe2b45099

Comment 9

[:ProgramFOX]

Attachment: Added shuffle's masks constants to the SIMD object

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8479013 [details] [diff] [review]
Added shuffle's masks constants to the SIMD object

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

Good job! Last round of review, and we should be good to go.

::: js/src/builtin/SIMD.cpp
@@ +16,5 @@
>  #include "jsapi.h"
>  #include "jsfriendapi.h"
>  
> +#include "builtin/SIMDShuffleMaskConstants.h"
> +

nit: no blank line here

::: js/src/builtin/SIMDShuffleMaskConstants.h
@@ +7,5 @@
> +#ifndef builtin_SIMDShuffleMaskConstants_h
> +#define builtin_SIMDShuffleMaskConstants_h
> +
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +    

nit: trailing whitespace

@@ +9,5 @@
> +
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +    
> +
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \

Could you #define SET_ALL_SHUFFLE_MASKS to call the macro below and use SET_ALL_SHUFFLE_MASKS in SIMD.cpp, rather than the macro below?

@@ +10,5 @@
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +    
> +
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \
> +    RootedValue mask##mask(cx, JS::Int32Value(value));      \

Can you wrap this entire macro in {}, so that it doesn't create too many temporary variables + rename the "mask" arg of the macro into "maskId"?

@@ +12,5 @@
> +
> +#define SET_SHUFFLE_MASK(macro, mask, maskStr, value)       \
> +    RootedValue mask##mask(cx, JS::Int32Value(value));      \
> +    JSObject::defineProperty(cx, SIMD, cx->names().mask,    \
> +                     mask##mask, nullptr, nullptr,          \

nit: align mask##mask with the parenthesis, same thing for JSPROP_READONLY below

::: js/src/vm/CommonPropertyNames.h
@@ +230,5 @@
>      macro(Symbol_iterator, Symbol_iterator, "Symbol.iterator") \
>      macro(Symbol_toPrimitive, Symbol_toPrimitive, "Symbol.toPrimitive") \
>      macro(Symbol_toStringTag, Symbol_toStringTag, "Symbol.toStringTag") \
> +    macro(Symbol_unscopables, Symbol_unscopables, "Symbol.unscopables") \
> +    /* SIMD shuffle masks */  \

no need for this comment, can you replace it with a blank line instead please?

Comment 11

[:ProgramFOX]

Attachment: Added shuffle's masks constants to the SIMD object

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8482869 [details] [diff] [review]
Added shuffle's masks constants to the SIMD object

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

Thanks for this contribution, looks good to me! Now that you have commit access L1 and can push to try, here are the next steps:
- fix the last nit (sorry about that)
- add "r=bbouvier" at the end of your commit message
- upload a new patch with the fixed nit and the edit commit message, carrying forward r+ (i.e. no need to ask again for review)
- push to try this last patch
- if try push goes back green (i.e. no red/orange related to your work), we'll set the checkin-needed keyword.
- one of our gentlemen sheriffs will see that keyword and push that to inbound on your behalf.
How does that sound?

::: js/src/builtin/SIMDShuffleMaskConstants.h
@@ +7,5 @@
> +#ifndef builtin_SIMDShuffleMaskConstants_h
> +#define builtin_SIMDShuffleMaskConstants_h
> +
> +#define COMMON_PROPERTY_NAMES_MACRO(macro, mask, maskStr, value) macro(mask, mask, maskStr)
> +

nit: sorry if i've been unclear. I'd like to have only blank line between the different macros in this file.

Comment 13

[:ProgramFOX]

Attachment: Added shuffle's masks constants to the SIMD object

Sounds good. I fixed the last nit and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=735317c88f6b

Comment 14

[:ProgramFOX]

The try push looks green to me, adding checkin-needed keyword.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/128ee74ae45d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/128ee74ae45d

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

\o/ Congrats ProgramFox on this patch! Keep up the good work ;)

Comment 18

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> \o/ Congrats ProgramFox on this patch!
Thanks!
> Keep up the good work ;)
Will certainly do that ;)