Closed Bug 993034 Opened 6 years ago Closed 5 years ago

SIMD: drive-by clean-ups

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bbouvier, Assigned: bbouvier)

Details

Attachments

(6 files)

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.
Attached patch 1 - ToType2Splinter Review
This one renames toType2 into toType, and makes it fallible, as one variant of toType calls toInt32 which is fallible.
Attachment #8402783 - Flags: review?(nmatsakis)
Attached patch 2 - Cleaning upSplinter Review
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.
Attachment #8402789 - Flags: review?(nmatsakis)
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.
Attachment #8402793 - Flags: review?(nmatsakis)
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).
Attachment #8402795 - Flags: review?(nmatsakis)
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.
Attachment #8402797 - Flags: review?(nmatsakis)
This one factors the reinterpret_cast(value.toObject().as<TypedObject>().typedMem()) out in a function, making all callers happier.
Attachment #8402799 - Flags: review?(nmatsakis)
Attachment #8402783 - Flags: review?(nmatsakis) → review+
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()
Attachment #8402789 - Flags: review?(nmatsakis) → review+
Attachment #8402793 - Flags: review?(nmatsakis) → review+
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()`?
Attachment #8402797 - Flags: review?(nmatsakis) → review+
Attachment #8402799 - Flags: review?(nmatsakis) → review+
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.
Attachment #8402795 - Flags: review?(nmatsakis) → review+
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
(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.
You need to log in before you can comment on or make changes to this bug.