Implement basic support for float32x4 as a typed object constructor

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla28
x86
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-][DocArea=JS])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
This bug is a preliminary step towards bug 904913. The idea is to create a new typed object data type `float32x4` that represents 4 float32s in sequence. As this is a value, and not a struct, references to a `float32x4` will be copied out of the containing structure, rather than creating aliases. The strawman specification and link to polyfill is here:

http://wiki.ecmascript.org/doku.php?id=strawman:simd_number

The `float32x4` type has four properties: 

- `x`, `y`, `z`, `w`

The `Float32x4Array` type can be implemented as a standard `ArrayType` wrapper around `float32x4`.

This work should also lay the foundation for future value types (like uint64 etc) that do not fit within a jsval.
(Assignee)

Updated

5 years ago
Blocks: 904913, 578700
(Assignee)

Comment 1

5 years ago
Posted patch Bug938728.diff (obsolete) — Splinter Review
Add float32x4 value type to typed objects. This is not fully compliant with what the eventual value types will look at, since float32x4s here are always objects (in the interpreter, at least, we should eventually be unboxing them in ion code as an unobservable optimization). Eventually float32x4 instances are supposed to be a new kind of value, like symbol -- however, even once that is done, the work here remains valid, basically the float32x4 function defined here is the wrapper type that will be instantiated whenever properties are accessed (like Number etc).

I'll probably post a follow up patch cleaning up the process for defining self-hosted getters, but I wanted to get this patch up now to avoid blocking intel.
Attachment #832942 - Flags: review?(till)
Comment on attachment 832942 [details] [diff] [review]
Bug938728.diff

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

Very nice! r=me with nits addressed and the mandatory request for license headers in test files fulfilled.

::: js/src/builtin/TypeRepresentation.cpp
@@ +82,5 @@
>  bool
> +TypeRepresentationHasher::matchFloat32x4s(Float32x4TypeRepresentation *key1,
> +                                          Float32x4TypeRepresentation *key2)
> +{
> +    return true;

Can you add a simple comment explaining why this is true? I stumbled over it and at first thought that this was still stubbed out.

@@ +375,5 @@
> +        if (!ptr)
> +            return nullptr;
> +        new(ptr) Float32x4TypeRepresentation();
> +
> +        return ptr->addToTableOrFree(cx, p);

Please use TypeRepresentationHelper::CreateSimple here, as you no doubt meant to do.

::: js/src/builtin/TypeRepresentation.h
@@ +198,5 @@
>  
>    private:
>      // so TypeRepresentation can call appendStringScalar() etc
>      friend class TypeRepresentation;
> +    // in order to call constructorb

what is "constructorb"?

::: js/src/builtin/TypedObject.cpp
@@ +1207,5 @@
> +};
> +
> +const JSFunctionSpec Float32x4Type::typeObjectMethods[] = {
> +    JS_FS_END
> +};

Do you know that these will contain entries in the near future? If not, let's not add them for now.

::: js/src/builtin/TypedObject.js
@@ +205,1 @@
>    return NewDerivedTypedDatum(this.typeObj, this.datum, this.offset);

Redundant line

@@ +247,5 @@
> +  // this reference to global. That bug has passed review and
> +  // should be landing soon, so before landing this patch, remove
> +  // this reference, which is clearly unsound with respect to
> +  // random users mucking about.
> +  return global.TypedObject.float32x4(x, y, z, w);

Don't forget removing this.

@@ +376,5 @@
> +  // have already matched. So if we get to this point, we're supposed
> +  // to "adapt" fromValue, but there are no legal adaptions.
> +  ThrowError(JSMSG_CANT_CONVERT_TO,
> +             typeof(fromValue),
> +             this.typeRepr.toSource())

no need for the line breaks

::: js/src/jit/IonBuilder.cpp
@@ +6497,5 @@
>          return true;
>  
>      switch (elemTypeReprs.kind()) {
> +      case TypeRepresentation::Float32x4:
> +        // FIXME -- load float32x4 into a MIRType_float32x4

Please file a bug and add the bug number.

@@ +8169,5 @@
>          return true;
>  
>      switch (fieldTypeReprs.kind()) {
> +      case TypeRepresentation::Float32x4:
> +        // FIXME -- load float32x4 into a MIRType_float32x4

ditto

@@ +8706,5 @@
>          return true;
>  
>      switch (fieldTypeReprs.kind()) {
> +      case TypeRepresentation::Float32x4:
> +        // FIXME -- store float32x4 into a MIRType_float32x4

ditto

::: js/src/vm/SelfHosting.cpp
@@ +1017,5 @@
> +                                             0, &funcValue))
> +        return false;
> +
> +    JS_ASSERT(funcValue.isObject() && funcValue.toObject().is<JSFunction>());
> +    RootedFunction func(cx, &funcValue.toObject().as<JSFunction>());

Everything before this point can be replaced by
RootedFunction func(cx, SelfHostedFunction(cx, selfHostedName));
if (!func)
  return false;
Attachment #832942 - Flags: review?(till) → review+

Comment 3

5 years ago
Comment on attachment 832942 [details] [diff] [review]
Bug938728.diff

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

It will be good if we implement the int32x4 data constructor also. I have pushed a commit into your bug904913-simd branch to implement int32x4 constructor.

::: js/src/builtin/TypeRepresentation.cpp
@@ +360,5 @@
> +/*static*/
> +JSObject *
> +Float32x4TypeRepresentation::Create(JSContext *cx)
> +{
> +        JSCompartment *comp = cx->compartment();

Indent at 4 instead of 8?

::: js/src/builtin/TypeRepresentation.h
@@ +239,5 @@
>      macro_(ScalarTypeRepresentation::TYPE_UINT8_CLAMPED, uint8_t, uint8Clamped)
>  
> +// Layout of a float32x4 in memory as a C struct
> +struct Float32x4 {
> +    float x;

For your information: We used "struct Float32x4 { float storage[4]; }" in the V8 and Dart implementation.

::: js/src/builtin/TypedObject.cpp
@@ +1207,5 @@
> +};
> +
> +const JSFunctionSpec Float32x4Type::typeObjectMethods[] = {
> +    JS_FS_END
> +};

To answer Till's comment: We could use float32x4.bool and float32x4.splat to construct a float32x4 value object.

@@ +1212,5 @@
> +
> +const JSPropertySpec Float32x4Type::typedObjectProperties[] = {
> +    // The x, y, z, and w properties are self-hosted and sadly
> +    // there is no nice macro for declaring that here.
> +    JS_PS_END

There is another getter in the prototype. Float32x4.prototype.signMask.
(Assignee)

Comment 4

5 years ago
Thanks Till, a few quick comments --

> Comment on attachment 832942 [details] [diff] [review]
> @@ +375,5 @@
> > +        if (!ptr)
> > +            return nullptr;
> > +        new(ptr) Float32x4TypeRepresentation();
> > +
> > +        return ptr->addToTableOrFree(cx, p);
> 
> Please use TypeRepresentationHelper::CreateSimple here, as you no doubt
> meant to do.

I did mean to, but I found I could not, because the constructor for `Float32x4TypeRepresentation` doesn't take any argument. I suppose I could give it a dummy argument (yuck...) or maybe find a more clever way of parameterizing. I originally had a kind of "meta type representation" like ScalarTypeRepresentation but it seemed like it was working out cleaner just making Float32x4 it's own kind. I'll have to examine if that remains true with `Int32x4`. In any case,  left `CreateSimple` in place because it'll be useful for reference types (bug 898359). I plan to land that first, so when I rebase this patch I'll have to remember to use it. :)
(Assignee)

Comment 5

5 years ago
Posted patch Bug938728.diff (obsolete) — Splinter Review
Incorporate till comments, r=nmatsakis
Attachment #832942 - Attachment is obsolete: true
Attachment #8335028 - Flags: review+
(Assignee)

Comment 6

5 years ago
Posted patch Bug938728-Part2.diff (obsolete) — Splinter Review
Make it convenient to declare self-hosted properties
Attachment #8335029 - Flags: review?(till)
(Assignee)

Comment 7

5 years ago
Posted patch Bug938728-Part3.diff (obsolete) — Splinter Review
Generalize float32x4 to support int32x4 as well
Attachment #8335030 - Flags: review?(till)

Comment 8

5 years ago
I have a comment on the type generalization.

We distinguish the Float32x4 and Int32x4 as two types in the V8 implementation. The memory layout of the Float32x4 is [float32x4_map, float32[4]]. When unboxing this object into a XMM register, we first check whether the object is a heap object, and then check whether its map is float32x4_map, and finally unbox the object into a register by "movups xmm_reg, float32x4_object + kPointSize" and mark this XMM register with a representation Representation::kFLOAT32x4. We need two type representation (kFLOAT32x4 and kINT32x4) for XMM registers as we might deoptimize and we need to reconstruct the float32x4 or int32x4 object from a raw XMM register.

From reading the codes, it seems that if we generalize, we need to access type() of TypeRepresentation for float32x4 and int32x4 when unboxing. Will it introduce an overhead? I assumed the TypeRepresentation and 4 floats are continuous in the memory space.
(Assignee)

Comment 9

5 years ago
(In reply to haitao from comment #8)
> From reading the codes, it seems that if we generalize, we need to access
> type() of TypeRepresentation for float32x4 and int32x4 when unboxing. Will
> it introduce an overhead? I assumed the TypeRepresentation and 4 floats are
> continuous in the memory space.

There should be no overhead -- in jitted code we will rely on type inference, which tells us the TypeRepresentation* of the value, so we will inspect the type during compilation, but don't need to inspect the type at all during runtime (type inference also handles inserting guards at function boundaries and other places they may be needed).

If we DID have to inspect the type representation dynamically for some reason, perhaps as part of an IC which we don't yet, we will don't have to check the type field (or the kind field, for that matter). Since type representations are canonicalized, we can simply check the pointer of the type representation itself rather than examining its contents (or, more likely, the type object). However, in such cases we wouldn't be able to do SIMD optimizations anyway, since that implies that multiple types of objects are flowing through the location.
(Assignee)

Comment 10

5 years ago
(In reply to Niko Matsakis [:nmatsakis] from comment #9)
> ... we will don't have to check the type field (or the kind field, for that matter) ...

Correction: we *still* don't have to...
Comment on attachment 8335029 [details] [diff] [review]
Bug938728-Part2.diff

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

Nice!

::: js/src/jsapi.cpp
@@ +3285,5 @@
>      RootedObject obj(cx, objArg);
>      bool ok;
>      for (ok = true; ps->name; ps++) {
> +        // If you have self-hosted getter/setter, you can't have a native one.
> +        JS_ASSERT_IF(ps->selfHostedGetter, !ps->getter.op && !ps->setter.op);

Can you move this into the `if (ps->selfHostedGetter)` block and make it an unconditional assert?

@@ +3290,5 @@
> +
> +        // If you have a self-hotsed setter, you best have a self-hosted getter.
> +        JS_ASSERT_IF(ps->selfHostedSetter, ps->selfHostedGetter);
> +
> +        if (ps->selfHostedSetter || ps->selfHostedGetter) {

No need to check for ps->selfHostedSetter here.
Attachment #8335029 - Flags: review?(till) → review+
Comment on attachment 8335030 [details] [diff] [review]
Bug938728-Part3.diff

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

Beautiful.

Naturally, the test license headers should be added.

::: js/src/builtin/TypeRepresentation.cpp
@@ +373,2 @@
>  {
> +    return TypeRepresentationHelper::CreateSimple<X4TypeRepresentation>(cx, type);

nice

::: js/src/builtin/TypedObject.js
@@ +248,5 @@
> +    // FIXME bug 917454 contains the function we need to eliminate
> +    // this reference to global. That bug has passed review and
> +    // should be landing soon, so before landing this patch, remove
> +    // this reference, which is clearly unsound with respect to
> +    // random users mucking about.

Don't forget this.

@@ +634,4 @@
>  }
>  
> +function X4LaneString(lane) {
> +  var laneStrings = ["x", "y", "z", "w"];

you can move this out of the function and just refer to it. I don't see why it would collide with anything, and it will be lazy-cloned like any other self-hosted value. OTOH, it doesn't matter much because it's only used for exceptions. Up to you.

::: js/src/builtin/TypedObjectConstants.h
@@ +79,5 @@
>  #define JS_SCALARTYPEREPR_FLOAT64       7
>  #define JS_SCALARTYPEREPR_UINT8_CLAMPED 8
>  
>  // These constants are for use exclusively in JS code.  In C++ code,
> +// prefer X4TypeRepresentation::TYPE_INT8 etc, since that allows

Please make this TYPE_INT32: referring to a type we don't actually have is confusing.
Attachment #8335030 - Flags: review?(till) → review+

Comment 13

5 years ago
Niko, thanks for the explanation, as usual.
(In reply to Niko Matsakis [:nmatsakis] from comment #9)
> (In reply to haitao from comment #8)
> > From reading the codes, it seems that if we generalize, we need to access
> > type() of TypeRepresentation for float32x4 and int32x4 when unboxing. Will
> > it introduce an overhead? I assumed the TypeRepresentation and 4 floats are
> > continuous in the memory space.
> 
> There should be no overhead -- in jitted code we will rely on type
> inference, which tells us the TypeRepresentation* of the value, so we will
> inspect the type during compilation, but don't need to inspect the type at
> all during runtime (type inference also handles inserting guards at function
> boundaries and other places they may be needed).
> 
> If we DID have to inspect the type representation dynamically for some
> reason, perhaps as part of an IC which we don't yet, we will don't have to
> check the type field (or the kind field, for that matter). Since type
> representations are canonicalized, we can simply check the pointer of the
> type representation itself rather than examining its contents (or, more
> likely, the type object). However, in such cases we wouldn't be able to do
> SIMD optimizations anyway, since that implies that multiple types of objects
> are flowing through the location.
(Assignee)

Comment 14

5 years ago
(In reply to Niko Matsakis [:nmatsakis] from comment #9)
> Since type representations are canonicalized, we can simply check the pointer of the
> type representation itself rather than examining its contents (or, more
> likely, the type object).

I realized that this final parenthetical comment is incorrect, we'd be unlikely
to use the type objects as the basis for an IC, but this doesn't really change
the overall point of the comment that no runtime ovehead is introduced.
(Assignee)

Comment 16

5 years ago
r=till
Attachment #8335028 - Attachment is obsolete: true
Attachment #8335029 - Attachment is obsolete: true
Attachment #8335030 - Attachment is obsolete: true
Attachment #8335777 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6015e057d116
Assignee: nobody → nmatsakis
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [qa-]
Keywords: dev-doc-needed
Whiteboard: [qa-] → [qa-][DocArea=JS]
You need to log in before you can comment on or make changes to this bug.