Closed Bug 1225605 Opened 4 years ago Closed 4 years ago

SIMD: a few interpreter cleanups

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files)

While looking at the patches for the boolean vectors, I saw a few things that could be cleaned up. Posting in this bug.
We use struct as the SIMD descriptors, then we use these structs in templates to generate all the different functions for each type.

In these structs:
- the toType(Elem) variant isn't actually used, so let's remove it.
- toType(JSContext*, HandleValue, Elem*) is referred to as [[Cast]] in the spec, let's rename it Cast
- no reason setReturn should take a CallArgs argument; instead, let's rename it ToValue and make it create a new JSValue from the given parameter
Attachment #8688625 - Flags: review?(jolesen)
The boolean vectors replace these much better. Plus, this patch is a good opportunity to see all the places affected when you want to implement a SIMD function in the interpreter and optimize it in Ion ;-)
Attachment #8688626 - Flags: review?(jolesen)
I saw an implicit dependency between the order of declarations in SimdTypeDescr::Type and the indexes of the static SimdLanes/SimdSizes arrays. The original macro also has two useless parameters, so let's recycle them to make this better, plus add some assertions by the way.
Attachment #8688627 - Flags: review?(jolesen)
Comment on attachment 8688625 [details] [diff] [review]
1. Rename a few things

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

This seems like a good idea, following the spec more closely.

All of these type descriptor structs are conforming to the same "concept" (à la C++17). It would be really helpful to document the concept and what the individual methods mean. (Only do this once, no need to repeat it for all the types.)

For extra credit, use C++17 concept syntax in the comment ;-)
Attachment #8688625 - Flags: review?(jolesen) → review+
Comment on attachment 8688626 [details] [diff] [review]
2. Remove non standard .bool function

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

Nice!

::: js/src/tests/ecma_7/SIMD/select-bitselect.js
@@ +13,5 @@
>  
>  function getMask(i, maskLength) {
>      var args = [];
>      for (var j = 0; j < maskLength; j++) args.push(!!((i >> j) & 1));
> +    args = args.map(x => !!x ? -1 : 0);

How about simply:

  ... args.push(((i >> j) & 1) ? -1 : 0);

(Or just leave it. I know this is changing again with bug 116971.)
Attachment #8688626 - Flags: review?(jolesen) → review+
Comment on attachment 8688627 [details] [diff] [review]
3. Make the dependency between SimdTypeDescr::Type and SimdLanes/SimdSizes clearer

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

Looks good!

::: js/src/builtin/TypedObject.cpp
@@ +427,5 @@
>  
>  int32_t
>  SimdTypeDescr::size(Type t)
>  {
> +    MOZ_ASSERT(t <= SimdTypeDescr::Type::LAST_TYPE);

Two-sided range check for the price of one:

MOZ_ASSERT((unsigned)t <= SimdTypeDescr::Type::LAST_TYPE);

::: js/src/builtin/TypedObject.h
@@ +356,5 @@
>  
>      SimdTypeDescr::Type type() const {
> +        auto t = (SimdTypeDescr::Type) getReservedSlot(JS_DESCR_SLOT_TYPE).toInt32();
> +        MOZ_ASSERT(t <= LAST_TYPE);
> +        return t;

Good idea with the assertion, but I wonder if this would trigger Clang's tautology warnings at some point?

Alternative impl:

uint32_t t = getReservedSlot(JS_DESCR_SLOT_TYPE).toInt32();
MOZ_ASSERT(t <= LAST_TYPE);
return (SimdTypeDescr::Type)t;

Note that the uint32_t here also catches eventual negative numbers.

::: js/src/builtin/TypedObject.js
@@ +144,5 @@
>  
>  function TypedObjectGetSimd(descr, typedObj, offset) {
>    var type = DESCR_TYPE(descr);
>    switch (type) {
> +  case JS_SIMDTYPEREPR_FLOAT32X4:

Nice. This is much clearer.
Attachment #8688627 - Flags: review?(jolesen) → review+
Thank you Jakob for the reviews. Adding dev-doc-needed for the removal of the bool methods.
Keywords: dev-doc-needed
leave-open: there are a few other patches coming in.
Keywords: leave-open
I've read about C++17 concepts, but the only associated wording I've used in this comment relates to constraints and requires ;-) Hope this documents more clearly what this is doing.
Attachment #8689993 - Flags: review?(jolesen)
After your comment on one of the other bug's patches (referring to a magic number 37 that got modified to 40), I've dig into this. It appears that this magic constant is the number of Value slots on a Global object, so it shouldn't grew because of SimdTypeDescr, which are, as of before this patch, stored on the global.

Instead, we can store them on the global SIMD object, which *has* to be there anyways if we're using SIMD. It adds an indirection on runtime, but we expect the interpreter to be slow anyways, so it is fine.

This is nice to do this, because:
1) we don't have to care about modifying this magic value anymore
2) the size of the Global object isn't tied to the number of SIMD types. As we have a *lot* of Global objects, it should be quite a win, and maybe jmaher will show up and be happy because of this change ;)
3) it removes a lot of code looking almost the same
4) it removes one constraint in the concept :)
Attachment #8689997 - Flags: review?(jolesen)
We don't really need the SimdSize/SimdLanes arrays, these can be implemented with switches instead.

Moreover, SIMD.js is fixed size as of today, so SimdSize should just return the only possible value and crash if we run into an unknown SIMD type descriptor.

The lane() function and slot are actually unused, let's just remove them.
Attachment #8689999 - Flags: review?(jolesen)
Blocks: 1160971
Comment on attachment 8689993 [details] [diff] [review]
4. Document the SIMD classes in SIMD.h

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

Perfect!
Attachment #8689993 - Flags: review?(jolesen) → review+
Comment on attachment 8689997 [details] [diff] [review]
5. Move type descriptors onto the SIMD global object

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

I don't completely understand the implications here, but the code looks clean.
Attachment #8689997 - Flags: review?(jolesen) → review+
Comment on attachment 8689999 [details] [diff] [review]
6. Simplify size() / alignment()

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

Looks good!

::: js/src/builtin/TypedObject.h
@@ -326,5 @@
>          return getReservedSlot(JS_DESCR_SLOT_TYPROTO).toObject().as<TypedProto>();
>      }
>  };
>  
> -#define JS_FOR_EACH_SIMD_TYPE_REPR(macro_)                    \

Nice. Getting rid of these macros makes the code much more readable. It doesn't seem worthwhile for so few types.
Attachment #8689999 - Flags: review?(jolesen) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cd57cd70de74
https://hg.mozilla.org/mozilla-central/rev/cbd0b3d73d3b
https://hg.mozilla.org/mozilla-central/rev/cf597c8a51f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Removed the bool method from the docs. More updates for the Bool vectors to come, though.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD
You need to log in before you can comment on or make changes to this bug.