1066020 - Interpreter: Add JSConstIntegerSpec and use it for SIMD shuffle masks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

When asking till for the best way to define permanent properties on a stdlib object (namely the global SIMD object) while looking for ways to reduce memory in bug 1062907, he told me the best way would be to use a JSConstPropertySpec. As there is already somebody like this for double properties (JSConstDoubleSpec), we can templatize it for int32 and use it afterwards when defining the SIMD masks.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Add JSConstIntegerSpec to jsapi

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Use JSConstIntegerSpec for defining SIMD shuffle masks in the interpreter

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8487876 [details] [diff] [review]
Use JSConstIntegerSpec for defining SIMD shuffle masks in the interpreter

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

::: js/src/builtin/SIMD.cpp
@@ +374,5 @@
>  
> +static JSConstIntegerSpec SHUFFLE_MASKS[] = {
> +#define ADD_INT32_SPEC(name, val) {val, name, 0, {0,0,0}},
> +    FOR_EACH_SIMD_SHUFFLE_MASK(ADD_INT32_SPEC)
> +    {0,0,0,{0,0,0}}

(fwiw, i've added an #undef ADD_INT32_SPEC after this line)

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8487875 [details] [diff] [review]
Add JSConstIntegerSpec to jsapi

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

Nice and simple, thanks. r=me with the requested name change applied.

::: js/src/jsapi.cpp
@@ +3218,5 @@
>      return nobj;
>  }
>  
> +static inline Value
> +CreateFromScalar(double x)

"Create" is too ambiguous, I think. How about "ValueFromScalar"?

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 2 - refactor

As discussed on IRC:
- removed the last two fields of the JSConstScalarSpec as they're not needed
- inverted order of name and value, as it makes more sense to read the name before reading the value
- not asking jorendorff for review as it isn't probably a widely used feature

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Part 3: Use JSConstIntegerSpec for defining SIMD shuffle masks in the interpreter

Also addresses comments made on irc:
- removes SIMDShuffleMasks*.h

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8487944 [details] [diff] [review]
Part 2 - refactor

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

Mmh, so much nicer. Sorry for the slow review.

::: js/src/jsapi.cpp
@@ +3233,5 @@
>  static bool
>  DefineConstScalar(JSContext *cx, HandleObject obj, const JSConstScalarSpec<T> *cds)
>  {
>      bool ok;
> +    unsigned attrs = JSPROP_READONLY | JSPROP_PERMANENT;

Might just as well move this down to where it's used. This isn't ancient-times C anymore, after all. Same for `ok`, though I don't feel strongly about that.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8487946 [details] [diff] [review]
Part 3: Use JSConstIntegerSpec for defining SIMD shuffle masks in the interpreter

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

What's not to like about this?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the reviews! We could actually get rid of the bool ok in patch 2, by just making the control flow more explicit.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8a281ea111f7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1cbab2a7889
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d76097710bfb

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a281ea111f7
https://hg.mozilla.org/mozilla-central/rev/c1cbab2a7889
https://hg.mozilla.org/mozilla-central/rev/d76097710bfb