993034 - SIMD: drive-by clean-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

While reading the SIMD code for understanding the API, I saw that the code could benefit from some clean ups. If you don't want to take all of them, that's fine; however I found them to make the code way more readable and logical.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1 - ToType2

This one renames toType2 into toType, and makes it fallible, as one variant of toType calls toInt32 which is fallible.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2 - Cleaning up

This is the main patch. It tries to respect more SM's coding style, does some manual LICM at some places, replaces one series of instructions by a loop, deletes unnecessary parenthesis in conditions, takes advantage of local typedef to reduce code size, and so on.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3 - Bogus pointer

I don't get why this was working. orInt is an int32_t[] === int32_t*, so taking its address should return a int32_t**, which was then casted into float*, making types non consistent. This patch makes it cleaner and avoids the 3 unnecessary array slots.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4 - Avoid Macros as much as possible

This one deletes the two gigantic LANE and SIGNMASK macros, replacing them with templated functions. We still need a LANE_ACCESSOR macro, because a macro apparently cannot take a templated method that has several template parameters (because of the comma).

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5 - IsVectorObject

Having ObjectIsVector<V> is nice, but most callers are actually testing | value.isObject() && ObjectIsVector<V>(value) |, so let's just factor that too and have our conditions skinnier.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 6 - TypedObjectMemory

This one factors the reinterpret_cast(value.toObject().as<TypedObject>().typedMem()) out in a function, making all callers happier.

Comment 7

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8402789 [details] [diff] [review]
2 - Cleaning up

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

::: js/src/builtin/SIMD.cpp
@@ +542,2 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    if (argc != 1 && argc != 2) {

Nit: I'm told using argc is deprecated, and we ought to favor args.length()

Comment 8

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8402797 [details] [diff] [review]
5 - IsVectorObject

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

::: js/src/builtin/SIMD.cpp
@@ +354,5 @@
>  
>      RootedValue SIMDValue(cx, ObjectValue(*SIMD));
>  
>      // Everything is set up, install SIMD on the global object.
> +    if (!JSObject::defineProperty(cx, global, cx->names().SIMD, SIMDValue, nullptr, nullptr, 0)) {

nit: remove the `{...}`

@@ +391,5 @@
>  }
>  
>  template<typename V>
> +static bool
> +IsVectorObject(HandleValue v)

Why not just merge these two functions together? Why keep the distinct `ObjectIsVector()`?

Comment 9

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8402795 [details] [diff] [review]
4 - Avoid Macros as much as possible

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

We actually had templates initially and moved to #define's -- I think the motivator was wanting to be able to have simple names we can reference from the MCallOptimize code. I too prefer editing `template<>` code to macros, though, so another option is to have macros declare a shallow wrapper that just calls the `template` function.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the reviews!

(In reply to Niko Matsakis [:nmatsakis] from comment #7)
> Comment on attachment 8402789 [details] [diff] [review]
> 2 - Cleaning up
> Nit: I'm told using argc is deprecated, and we ought to favor args.length()

Fixed.

(In reply to Niko Matsakis [:nmatsakis] from comment #8)
> Comment on attachment 8402797 [details] [diff] [review]
> 5 - IsVectorObject
> Why not just merge these two functions together? Why keep the distinct
> `ObjectIsVector()`?
Indeed, nice catch.

(In reply to Niko Matsakis [:nmatsakis] from comment #9)
> Comment on attachment 8402795 [details] [diff] [review]
> 4 - Avoid Macros as much as possible
> We actually had templates initially and moved to #define's -- I think the
> motivator was wanting to be able to have simple names we can reference from
> the MCallOptimize code. I too prefer editing `template<>` code to macros,
> though, so another option is to have macros declare a shallow wrapper that
> just calls the `template` function.
Alright. I kept it as is. If we ever want simpler names again, it won't be too big a change.

Running a try build to be sure (mixing templates and macros as in patch 4 could make some compilers angry)
https://tbpl.mozilla.org/?tree=Try&rev=edf5a6d21a0c

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> (mixing templates and macros as in patch 4
> could make some compilers angry)
> https://tbpl.mozilla.org/?tree=Try&rev=edf5a6d21a0c

Sometimes one doesn't like to be right. Reverting back to (more) shallow wrappers macros that call the templated method, as suggested in comment 9, is enough.

https://tbpl.mozilla.org/?tree=Try&rev=1704da84aa3b (OS X reds are caused by bug 996621)

I'll land it as soon as the OS X build returns green and the tree opens.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

OK, infra red still present on B2G OS X Opt, but everywhere else is all green, so let's land it

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce48eb7d960
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/810593c39953
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b1f10cb214a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87e9fc86154e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/330cc814b05b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c402f8b74fc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ce48eb7d960
https://hg.mozilla.org/mozilla-central/rev/810593c39953
https://hg.mozilla.org/mozilla-central/rev/6b1f10cb214a
https://hg.mozilla.org/mozilla-central/rev/87e9fc86154e
https://hg.mozilla.org/mozilla-central/rev/330cc814b05b
https://hg.mozilla.org/mozilla-central/rev/9c402f8b74fc