1102870 - Odin: Simplify the AsmJSHeapAccess::ViewType vs Scalar::Type situation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

After landing of atomic's patches (bug 1073096), lowering and platform-specific codegen use much more Scalar::Type, while my patch in the load/store bug (bug 1079361) replaced former uses by AsmJSHeapAccess::ViewType, which is an enum inheriting Scalar::Type, but that also contains Float32x4 and Int32x4.

Right now, there are a few unfortunate situations:
- Atomics code uses a lot Scalar::Type and calls interfaces that expect Scalar::Type. During rebasing, I just made sure that the AsmJSHeapAccess::ViewType in the operations were not Float32x4 and Int32x4 (by comparing the enum values; could be better), and converted back from ViewType to Scalar::Type, to not block landing.
- x86 has visit{Load,Store}TypedArrayElementStatic, which calls interfaces that are expecting AsmJSHeapAccess::ViewType while the LIR / MIR instructions contain info for a Scalar::Type. This was handled by converting the Scalar::Type to a ViewType in codegen, but that's unfortunate.

I'd like to keep it simple and just have one enum used everywhere.

A simple solution would be to add Float32x4 and Int32x4 to Scalar::Type, but these are tightly coupled to TypedArray. However, there seems to be Float32x4Array and Int32x4Array in the polyfill; not sure how much this is needed / useful. If we plan to implement these in JS land at least, having these types there would make sense.

Another solution would be to extract the enum from AsmJSHeapAccess, to have it not dependent to AsmJS anymore, and use it in Load/StoreTypedArrayElementStatic as well as in AsmJSHeapAccess. Any thoughts?

Comment 1

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #0)

> Another solution would be to extract the enum from AsmJSHeapAccess, to have
> it not dependent to AsmJS anymore, and use it in
> Load/StoreTypedArrayElementStatic as well as in AsmJSHeapAccess.

This has my vote, if extending Scalar::Type is unnatural for other reasons.

I don't know if this matters, but before long we must add or ensure that SIMD data can be loaded from and stored to shared memory arrays as well (though not atomically), so any asymmetry in the current code arising from that not being implemented may perhaps be cleaned up.  (Ideally SIMD.load/store should just apply to shared-memory arrays, in the way that one can use the array[access] operator to access scalar data.)

Comment 2

[Luke Wagner [:luke]]

Although it doesn't like it will currently be the plan, there is a possibility we'll get Int32x4Array/Float32x4Array in the future.  Also, I expect one can make a ArrayType(SIMD.float32x4, N).  So perhaps it isn't so bad to add Int32x4/Float32x4 to Scalar::Type anyway.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Add Float32x4 and Int32x4 to the list of Scalar::Types

When / If implementing Float32x4Array and Int32x4Array, one will just need to
grep for Scalar::TypeX4 and find some places to implement.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Replace AsmJSHeapAccess::ViewType by Scalar::Type

This is pretty mechanic.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1fabf0b692e9

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Add Float32x4 and Int32x4 to the list of Scalar::Types

So this patch was breaking everything, because there are implicit relations
between Scalar::Type enum values and TypedArray. As a matter of fact, having
Float32x4 and Int32x4 before Uint8Clamped had the unfortunate consequence that
the Uint8Clamped typed array became undefined. I've added a comment about this
in Scalar::Type.

The way to fix this was to insert dummy empty Class in
TypedArrayObject::{classes,protoClasses}, but this bug isn't about implementing
SIMD typed arrays, so I've just put the two enum values after Scalar::TypeMax.
Try run coming, to confirm whether it's a good idea or not.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8a877f5a6bf4

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8529909 [details] [diff] [review]
Replace AsmJSHeapAccess::ViewType by Scalar::Type

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +2771,1 @@
>                     NeedsBoundsCheck chk)

I think this all fits on one line now.

@@ +2802,1 @@
>                           NeedsBoundsCheck chk)

Same comment.

@@ +2814,1 @@
>                                             MDefinition *oldv, MDefinition *newv, NeedsBoundsCheck chk)

I think more can fit on the first line now.

@@ +2828,1 @@
>                                   MDefinition *ptr, MDefinition *v, NeedsBoundsCheck chk)

Same comment.

::: js/src/jit/shared/Assembler-shared.h
@@ +739,5 @@
>      uint32_t offset_;
>  #if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64)
>      uint8_t cmpDelta_;  // the number of bytes from the cmp to the load/store instruction
>      uint8_t opLength_;  // the length of the load/store instruction
> +    Scalar::Type viewType_;

Could you rename all these 'viewType' (and 'vt') variable/parameter/method names to just 'type'?

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +266,5 @@
>  }
>  
>  template<typename T>
>  void
> +CodeGeneratorX86::loadViewTypeElement(Scalar::Type vt, const T &srcAddr, const LDefinition *out)

This name no longer seems apt.  How about CodeGeneratorX86::load?

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8530314 [details] [diff] [review]
Add Float32x4 and Int32x4 to the list of Scalar::Types

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

::: js/src/jsfriendapi.h
@@ +1435,5 @@
>       * Treat the raw data type as a uint8_t.
>       */
>      Uint8Clamped,
>  
> +    TypeMax,

This is no longer the maximal type, but the maximal type of an array view.  Perhaps MaxTypedArrayViewType and move comment on Float32x4 to be on this enumerator instead?

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8530314 [details] [diff] [review]
Add Float32x4 and Int32x4 to the list of Scalar::Types

Since I'm not a primary typed array hacker, I probably shouldn't be r+'ing this patch without at least pinging someone.

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8530314 [details] [diff] [review]
Add Float32x4 and Int32x4 to the list of Scalar::Types

I'm fine with this given the current state of things. But I'll echo Luke's review comment about renaming TypeMax.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Renaming patch

So this is the renaming patch (to be folded in part 1). As it's bigger than expected, asking review to sfink for confirming it's fine as is.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Try build
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=430652b23f59

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8531814 [details] [diff] [review]
Renaming patch

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

::: dom/canvas/WebGLContext.h
@@ +1254,5 @@
>      void MakeContextCurrent() const;
>  
>      // helpers
>  
> +    // If `isArrayType is MaxTypedArrayViewType, it means no array.

s/`isArrayType/jsArrayType/

::: js/src/jsfriendapi.h
@@ +1439,5 @@
>      /*
>       * SIMD types don't have their own TypedArray equivalent, for now.
>       */
> +    MaxTypedArrayViewType,
> +

Both the previous and new versions are named a little oddly -- it's called "max", but it's actually one past the max (or equivalently, it's the number of types).

But I don't have a better suggestion, so I guess this is fine. I'm not sure why you moved it below the comment, though; seems like it should still be above.

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7cfe0628b5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6708bccc56

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a7cfe0628b5
https://hg.mozilla.org/mozilla-central/rev/9f6708bccc56