Ivan Jibaja Assignee
	Description • 11 years ago

Matching polyfill specification
https://github.com/johnmccutchan/ecmascript_simd

WIP

	Niko Matsakis [:nmatsakis]
	Comment 2 • 11 years ago

Comment on attachment 8342078 [details] [diff] [review]
patch WIP

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

Some comments.

::: js/src/builtin/SIMD.cpp
@@ +32,5 @@
> +		JS_StrictPropertyStub,   /* setProperty */
> +		JS_EnumerateStub,
> +		JS_ResolveStub,
> +		JS_ConvertStub,
> +		NULL,

Please label what fields these correspond to in a comment, and use nullptr instead of NULL.

@@ +42,5 @@
> +};
> +
> +
> +bool
> +SIMDObject::construct(JSContext *cx, unsigned int argc, jsval *vp)

You don't want to provide a construct callback, as `new SIMD()` is meaningless.

@@ +64,5 @@
> +
> +bool
> +SIMDObject::is(const Value &v)
> +{
> +	return v.isObject() && v.toObject().hasClass(&class_);

toObject().is<SIMDObject>() is preferred, I think, though it amounts to the same thing.

@@ +75,5 @@
> +	if (!SIMD)
> +		return NULL;
> +
> +	RootedFunction ctor(cx,
> +			global->createConstructor(cx, SIMDObject::construct, cx->names().SIMD, 4));

What is `ctor` used for? Apparently nothing? I think you can just get rid of it.
Instead, you'll want to install float32x4 and int32x4 objects as properties of SIMD.
They can probably just be instances of JSObject with Object.prototype as their
prototype. The main thing is install the self-hosted
helper functions. There is a convenient helper for doing this.

Here is an example:

Defining the array with methods (look for JS_SELF_HOSTED_FN, in particular):

http://dxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#2923

Installing the methods:

http://dxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#3091

@@ +85,5 @@
> +		return NULL;
> +	}
> +
> +	RootedValue SIMDValue(cx, ObjectValue(*SIMD));
> +	if(!JSObject::defineProperty(cx, global, cx->names().TypedObject,  SIMDValue, nullptr, nullptr, 0))

I suspect you mean cx->names().SIMD here. :)

	Niko Matsakis [:nmatsakis]
	Comment 4 • 11 years ago

ijibaja -- here is a rewrite that adds functional SIMD.float32x4.add and friends. I removed the Float32x4.cpp and Int32x4.cpp files and replaced them with self-hosted code (it'll ever so slightly complicate optimizing in ion, in that I don't think there is a pre-existing path for substituting specialized versions of calls to self-hosted fns, but it makes everything else easier). I also took the liberty of reformatting SIMD.cpp to be (more) in line with the SpiderMonkey style guidelines (which are generally enforced fairly rigorously).

	Ivan Jibaja Assignee
	Comment 5 • 11 years ago

Needs to be done: 

1) Need to be moved to be functions of their respective typed objects (not of the properties of SIMD object): splat, bool, zero
2) float32x4 and int32x4 constructors need to be added to the global space
3) add signMask property
4) Write tests for int32x4 (only tested the float32x4 functions)

	Ivan Jibaja Assignee
	Comment 6 • 11 years ago

Forgot to overwrite previous patch

	Ivan Jibaja Assignee
	Comment 7 • 11 years ago

Still needs to be done: 

2) float32x4 and int32x4 constructors need to be added to the global space (also the splat, bool, zero constructor functions of those objects)
3) add signMask property

	Ivan Jibaja Assignee
	Comment 10 • 11 years ago

all tests passing

	Ivan Jibaja Assignee
	Comment 11 • 11 years ago

Completely up to spec
All tests passing

	Ivan Jibaja Assignee
	Comment 12 • 11 years ago

Spec updated, int32x4.zero() function added

	Till Schneidereit [:till]
	Comment 13 • 11 years ago

Comment on attachment 8344069 [details] [diff] [review]
bug946042-diff.txt

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

This doesn't apply on tip, because various things have changed about how typed objects are handled. It's complex enough that I want to compile it instead of just looking at the diff in Bugzilla. I'm clearing the review flag for now and will do a review after you've rebased to current tip.

	Ivan Jibaja Assignee
	Comment 14 • 11 years ago

Rebased to current tip

	Till Schneidereit [:till]
	Comment 15 • 11 years ago

Comment on attachment 8344314 [details] [diff] [review]
bug946042-diff.txt

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

This looks very nice! Some comments I want to see addressed before giving an r+:

I think we'll want to keep this behind a flag for now, so please add the machinery for a ENABLE_JS_SIMD flag and add #ifdefs around all places where SIMD.h & co are used. You can look at ENABLE_YARR_JIT for an example of how to do this and where to add the #ifdefs. I have also noted the places where #ifdefs are needed, but I'm not 100% sure I caught all of them.

While I've commented extensively on js_InitTypedObject{Int,Float}32x4Class, it seems to me like they are fully redundant, as the classes should be accessed via the TypedObjectModule. If I'm correct about this, please just remove them completely instead of folding them into js_InitTypedObjectModuleObject as I suggested in TypedObject.cpp.

I've added some small nits to the first test file. Please apply those to all other tests.

Note also that I can't yet comment on the semantics of all of this as I'm not familiar with the draft spec. I'll read up on it for the next review, though.

::: js/src/Makefile.in
@@ +598,4 @@
>    $(srcdir)/builtin/ParallelArray.js \
>    $(srcdir)/builtin/String.js \
>    $(srcdir)/builtin/Set.js \
> +  $(srcdir)/builtin/SIMD.js \

#ifdef needed

::: js/src/builtin/SIMD.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * JS SIMD package.

Please add a link to the draft spec and an explanation of where which parts of the implementation can be found (i.e., mention that parts are self-hosted), and how SIMD relates to typed objects on a conceptual and an implementation level

@@ +9,5 @@
> + */
> +
> +#include "builtin/SIMD.h"
> +#include "jsapi.h"
> +

Move blank line above jsapi include

@@ +13,5 @@
> +
> +#include "jsfriendapi.h"
> +#include "jsobjinlines.h"
> +
> +// #include <xmmintrin.h>

Remove

@@ +49,5 @@
> +        JS_SELF_HOSTED_FN("clamp", "Float32x4Clamp", 3, 0),
> +        JS_SELF_HOSTED_FN("scale", "Float32x4Scale", 2, 0),
> +        JS_SELF_HOSTED_FN("toInt32x4", "Float32x4ToInt32x4", 1, 0),
> +        JS_SELF_HOSTED_FN("bitsToInt32x4", "Float32x4BitsToInt32x4", 1, 0),
> +

Remove blank line

@@ +92,5 @@
> +        nullptr,				/* finalize    */
> +        nullptr,				/* checkAccess */
> +        nullptr,				/* call        */
> +        nullptr,				/* hasInstance */
> +        nullptr,			    /* construct   */

Please fix the alignment of these comments

@@ +101,5 @@
> +SIMDObject::initClass(JSContext *cx, Handle<GlobalObject *> global)
> +{
> +    // Create SIMD Object.
> +    RootedObject SIMD(cx);
> +    SIMD = NewObjectWithClassProto(cx, &SIMDObject::class_, nullptr, global, SingletonObject);

You can inline this into the declaration of the SIMD var (i.e., `RootedObject SIMD(cx, NewObjectWithGivenProto(..))`

Note also the usage of NewObjectWithGivenProto instead of NewObjectWithClassProto. We want to get rid of all usages of the latter eventually

@@ +107,5 @@
> +        return nullptr;
> +
> +    // Create Float32x4 and Int32x4 object.
> +    RootedObject float32x4Proto(cx, global->getOrCreateObjectPrototype(cx));
> +    RootedObject int32x4Proto(cx, global->getOrCreateObjectPrototype(cx));

These are identical, so no need to have them both. Instead, do this once above the SIMD object creation, call it "objProto", and use it for all three object creations

@@ +111,5 @@
> +    RootedObject int32x4Proto(cx, global->getOrCreateObjectPrototype(cx));
> +    if (!float32x4Proto || !int32x4Proto)
> +        return nullptr;
> +    RootedObject float32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, float32x4Proto, global, SingletonObject));
> +    RootedObject int32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, int32x4Proto, global, SingletonObject));

Please watch out for the 100 columns line width limit

@@ +117,5 @@
> +        return nullptr;
> +
> +    // Define Float32x4 & Int32x4 functions.
> +    if (!JS_DefineFunctions(cx, int32x4, Int32x4Methods) ||
> +            !JS_DefineFunctions(cx, float32x4, Float32x4Methods))

Fix alignment and add braces

@@ +122,5 @@
> +        return nullptr;
> +
> +    // Install Float32x4 & Int32x4 object as a properties of SIMD object.
> +    if (!JS_DefineProperty(cx, SIMD, "float32x4", OBJECT_TO_JSVAL(float32x4),
> +            JS_PropertyStub, JS_StrictPropertyStub, 0) ||

Please align all arguments with the opening brace. I.e., "JS_PropertyStub" should be aligned with "cx"

@@ +124,5 @@
> +    // Install Float32x4 & Int32x4 object as a properties of SIMD object.
> +    if (!JS_DefineProperty(cx, SIMD, "float32x4", OBJECT_TO_JSVAL(float32x4),
> +            JS_PropertyStub, JS_StrictPropertyStub, 0) ||
> +        !JS_DefineProperty(cx, SIMD, "int32x4", OBJECT_TO_JSVAL(int32x4),
> +            JS_PropertyStub, JS_StrictPropertyStub, 0))

Same here

@@ +132,5 @@
> +
> +    // Everything is setup, install SIMD on the global object.
> +    RootedValue SIMDValue(cx, ObjectValue(*SIMD));
> +    if (!JSObject::defineProperty(cx, global, cx->names().SIMD,  SIMDValue,
> +            nullptr, nullptr, 0))

Same here

@@ +137,5 @@
> +    {
> +        return nullptr;
> +    }
> +
> +    global->setConstructor(JSProto_SIMD, SIMDValue);

Please move this above the above defineProperty to make the comment above that actually true

@@ +145,5 @@
> +JSObject *
> +js_InitSIMDClass(JSContext *cx, HandleObject obj)
> +{
> +    JS_ASSERT(obj->is<GlobalObject>());
> +    Rooted<GlobalObject *> global(cx, &obj->as<GlobalObject>());

RootedGlobal (and no: I don't know why we don't also have a HandleGlobal)

::: js/src/builtin/SIMD.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef SIMD_h
> +#define SIMD_h

add "builtin_" to the name here and at the end of the file

@@ +13,5 @@
> +#include "jsapi.h"
> +#include "jsobj.h"
> +
> +/*
> + * JS SIMD functions.

Please link to the draft spec on github here. Once we have a stable spec, we can link to that.

::: js/src/builtin/SIMD.js
@@ +1,1 @@
> +#include "TypedObjectConstants.h"

License header missing

@@ +1,2 @@
> +#include "TypedObjectConstants.h"
> +#include "TypedObjectSelfHosted.h"

Hmm, it's not ideal that these headers are included both here and in TypedObject.js: all the builtin/*.js files are concatenated during the build process. Maybe include them in Utilities.js so that they're guaranteed to be available.

@@ +1,3 @@
> +#include "TypedObjectConstants.h"
> +#include "TypedObjectSelfHosted.h"
> +

Please add a comment referencing the draft spec here

@@ +1,4 @@
> +#include "TypedObjectConstants.h"
> +#include "TypedObjectSelfHosted.h"
> +
> +function IsVectorDatum(val, type) {

Please assert that `type` is a JS_TYPEREPR_X4_KIND

@@ +6,5 @@
> +    return false;
> +  if (!ObjectIsTypedDatum(val))
> +    return false;
> +  var typeRepr = DATUM_TYPE_REPR(val);
> +  REPR_KIND(typeRepr);

Please remove

@@ +7,5 @@
> +  if (!ObjectIsTypedDatum(val))
> +    return false;
> +  var typeRepr = DATUM_TYPE_REPR(val);
> +  REPR_KIND(typeRepr);
> +  if (REPR_KIND(typeRepr) != JS_TYPEREPR_X4_KIND)

Isn't this check fully subsumed by the one below? If so, please remove

@@ +12,5 @@
> +    return false;
> +  return REPR_TYPE(typeRepr) == type;
> +}
> +
> +function Float32x4Zero() { 

Here and in the following 50 or so lines, there's lots of whitespace at the line ends. Please remove.

@@ +22,5 @@
> +  var T = GetTypedObjectModule(); 
> +  return T.int32x4(0.0,0.0,0.0,0.0); 
> +}
> +
> +function Int32x4Bool(x,y,z,w) {

Missing whitespace between arguments

@@ +27,5 @@
> +  var T = GetTypedObjectModule(); 
> +  return T.int32x4(x ? -1 : 0x0,
> +                     y ? -1 : 0x0,
> +                     z ? -1 : 0x0,
> +                     w ? -1 : 0x0);

Please fix alignment of these three lines

@@ +28,5 @@
> +  return T.int32x4(x ? -1 : 0x0,
> +                     y ? -1 : 0x0,
> +                     z ? -1 : 0x0,
> +                     w ? -1 : 0x0);
> +  

Remove blank line

@@ +31,5 @@
> +                     w ? -1 : 0x0);
> +  
> +}
> +
> +function Float32x4Scale(arg0, s) {

Please use speaking names for the arguments. Maybe "base" or "val" and "scaleFactor" or "factor"?

@@ +32,5 @@
> +  
> +}
> +
> +function Float32x4Scale(arg0, s) {
> +  if (!IsVectorDatum(arg0, JS_X4TYPEREPR_FLOAT32)) {

ifs with one-line conditions and bodies don't need braces

@@ +43,5 @@
> +  var T = GetTypedObjectModule(); 
> +  return T.float32x4(lane0*s, lane1*s, lane2*s, lane3*s);
> +}
> +
> +function Float32x4Clamp(arg0, lowerLimit, upperLimit) {

Speaking name instead of "arg0", please

@@ +44,5 @@
> +  return T.float32x4(lane0*s, lane1*s, lane2*s, lane3*s);
> +}
> +
> +function Float32x4Clamp(arg0, lowerLimit, upperLimit) {
> +  if (!IsVectorDatum(arg0, JS_X4TYPEREPR_FLOAT32)) {

No braces needed

@@ +68,5 @@
> +  cy = cy > ulY ? ulY : cy;
> +  cz = cz > ulZ ? ulZ : cz;
> +  cw = cw > ulW ? ulW : cw;
> +  var T = GetTypedObjectModule(); 
> +  return T.float32x4(cx,cy,cz,cw);

Missing whitespace between args

@@ +71,5 @@
> +  var T = GetTypedObjectModule(); 
> +  return T.float32x4(cx,cy,cz,cw);
> +}
> +
> +function Int32x4Select(t, trueValue, falseValue) {

Speaking name instead of "t", please

@@ +73,5 @@
> +}
> +
> +function Int32x4Select(t, trueValue, falseValue) {
> +   if (!IsVectorDatum(t, JS_X4TYPEREPR_INT32) || 
> +       !IsVectorDatum(trueValue, JS_X4TYPEREPR_FLOAT32)|| 

Whitespace missing before ||

@@ +74,5 @@
> +
> +function Int32x4Select(t, trueValue, falseValue) {
> +   if (!IsVectorDatum(t, JS_X4TYPEREPR_INT32) || 
> +       !IsVectorDatum(trueValue, JS_X4TYPEREPR_FLOAT32)|| 
> +       !IsVectorDatum(falseValue, JS_X4TYPEREPR_FLOAT32)) {

Brace on new line for multi-line conditions

@@ +86,5 @@
> +  var otf = Int32x4Or(tr,fr);
> +  return Int32x4BitsToFloat32x4(otf);
> +}
> +
> +#define DECLARE_SIMD_SPLAT(foo32, Foo32, FOO32, OPNAME) \

Speaking names instead of "foo*" here and in the macros below, please

@@ +87,5 @@
> +  return Int32x4BitsToFloat32x4(otf);
> +}
> +
> +#define DECLARE_SIMD_SPLAT(foo32, Foo32, FOO32, OPNAME) \
> +function Foo32##x4##OPNAME (arg0) { \

Same here: speaking names instead of "arg*" in all the macros

@@ +243,5 @@
> +#define OR(a,b) ((a) | (b))
> +#define NOT(a) (~a)
> +#define IMUL(a,b) std_Math_imul(a, b)
> +#define WITH(b) (b)
> +#define WITHFLAG(b) (b ? -1 : 0x0)

Please move all of these macros into a header. TypedObjectSelfHosted.h works for me, as long as you put them into an #ifdef ENABLE_JS_SIMD block

@@ +249,5 @@
> +// We need to ensure that the operations have float32 semantics.  I
> +// believe they should, because the data originates from a float32
> +// source and the result of op is coerced back to float32 as part of
> +// the float32x4 constructor. However, we could insert calls to
> +// Math.fround to be certain.

How about doing that in an #ifdef DEBUG block? Then you could change this comment from speculating to mentioning this assertion and why it holds.

@@ +279,5 @@
> +DECLARE_SIMD_CONVERT_BITWISE(float32, Float32, FLOAT32, BitsToInt32x4, int32)
> +
> +
> +// We need to ensure the operations have int32 semantics. Addition is
> +// fine but Math.imul is required for multiplication, I believe.

Please verify this belief and change things accordingly. At the very least, you should create a bug for it and reference it in a FIXME comment. Really, though, this shouldn't land without proper behavior in this regard

::: js/src/builtin/TypedObject.cpp
@@ +1267,5 @@
>  };
>  
> +const JSFunctionSpec js::Float32x4Defn::TypedObjectStaticMethods[] = {
> +    JS_SELF_HOSTED_FN("zero", "Float32x4Zero", 0, 0),
> +    JS_SELF_HOSTED_FN("splat", "Float32x4Splat", 1, 0),

Please swap lines around to keep symmetrical with Int32x4Defn (and alphabetical)

@@ +1637,5 @@
>      return !!proto;
>  }
>  
> +JSObject *
> +js_InitTypedObjectFloat32x4Class(JSContext *cx, HandleObject obj)

I think this function (and the equivalent for Int32x4 below) shouldn't exist. Their functionality could much easier be added to js_InitTypedObjectModuleObject with just another JSObject::defineProperty call for each one. In jsapi.h, you should get away with simply specifying js_InitTypedObjectModuleObject as the initializer for the two global properties.

I've added some comments to the code below, as there are some typical issues to be aware of, but you obviously don't need to address them when removing the functions.

@@ +1641,5 @@
> +js_InitTypedObjectFloat32x4Class(JSContext *cx, HandleObject obj)
> +{
> +    /*
> +     * This function is entered into the jsprototypes.h table
> +     * as the initializer for `float32x4`.

Please move this comment above the function

@@ +1644,5 @@
> +     * This function is entered into the jsprototypes.h table
> +     * as the initializer for `float32x4`.
> +     */
> +
> +    Rooted<GlobalObject *> global(cx, &obj->as<GlobalObject>());

You can use RootedGlobal here

@@ +1649,5 @@
> +
> +    // if the global TypedObject doesn't exist, initialize it
> +    RootedId id(cx, NameToId(cx->names().TypedObject));
> +    bool hasP;
> +    if (!JSObject::hasProperty(cx, global, id, &hasP) && !hasP)

This if condition is wrong. hasProperty returning false means that an error has occurred, upon which you'd have to abort this function. Additionally, you have to check the return value of js_InitTypedObjectModuleObject and return nullptr if it failed.

@@ +1653,5 @@
> +    if (!JSObject::hasProperty(cx, global, id, &hasP) && !hasP)
> +        js_InitTypedObjectModuleObject(cx,obj);
> +    JSObject *typedObj;
> +    typedObj = &(global->getTypedObjectModule());
> +    RootedObject typedObject(cx, typedObj);

No need to split this into two steps. You can just do
RootedObject typedObject(cx, &global->getTypedObjectModule());
(Note also the removal of braces after the &)

@@ +1657,5 @@
> +    RootedObject typedObject(cx, typedObj);
> +
> +    // get the float32x4 property from the globalTypedObject
> +    RootedValue float32x4Value(cx);
> +    if(!JSObject::getProperty(cx,typedObject, typedObject,

Missing space after "cx,"

@@ +1659,5 @@
> +    // get the float32x4 property from the globalTypedObject
> +    RootedValue float32x4Value(cx);
> +    if(!JSObject::getProperty(cx,typedObject, typedObject,
> +                              cx->names().float32x4,
> +                              &float32x4Value))

For multi-line conditions, you need braces around the body

@@ +1665,5 @@
> +    // install it as a global property
> +    if (!JSObject::defineProperty(cx, cx->global(), cx->names().float32x4,
> +                                  float32x4Value,
> +                                  nullptr, nullptr,
> +                                  JSPROP_READONLY | JSPROP_PERMANENT))

Braces needed

@@ +1671,5 @@
> +    return float32x4Value.toObjectOrNull();
> +}
> +
> +JSObject *
> +js_InitTypedObjectInt32x4Class(JSContext *cx, HandleObject obj)

This has pretty much the same issues as the function above

::: js/src/builtin/TypedObjectSelfHosted.h
@@ +1,1 @@
> +///////////////////////////////////////////////////////////////////////////

Please add the license header to this file

::: js/src/builtin/Utilities.js
@@ +52,5 @@
>  var std_Date_valueOf = Date.prototype.valueOf;
>  var std_Function_bind = Function.prototype.bind;
>  var std_Function_apply = Function.prototype.apply;
> +var std_Math_abs = Math.abs;
> +var std_Math_sqrt = Math.sqrt;

Please move down to keep the Math functions in alphabetical order

::: js/src/jsapi.cpp
@@ +55,4 @@
>  #include "builtin/ParallelArray.h"
>  #include "builtin/RegExp.h"
>  #include "builtin/TypedObject.h"
> +#include "builtin/SIMD.h"

add #ifdef

@@ +1232,5 @@
>  #if JS_HAS_UNEVAL
>      {js_InitStringClass,        EAGER_ATOM(uneval), OCLASP(String)},
>  #endif
> +    {js_InitSIMDClass,               EAGER_ATOM(SIMD), OCLASP(SIMD)},
> +    {js_InitTypedObjectModuleObject,   EAGER_ATOM(TypedObject), OCLASP(TypedObjectModule)},

No need to explicitly call this here: it's called inside both of the following functions

@@ +1233,5 @@
>      {js_InitStringClass,        EAGER_ATOM(uneval), OCLASP(String)},
>  #endif
> +    {js_InitSIMDClass,               EAGER_ATOM(SIMD), OCLASP(SIMD)},
> +    {js_InitTypedObjectModuleObject,   EAGER_ATOM(TypedObject), OCLASP(TypedObjectModule)},
> +    {js_InitTypedObjectInt32x4Class, EAGER_ATOM(int32x4), &js::X4Type::class_},

Use the OCLASP macro here and below

@@ +1234,5 @@
>  #endif
> +    {js_InitSIMDClass,               EAGER_ATOM(SIMD), OCLASP(SIMD)},
> +    {js_InitTypedObjectModuleObject,   EAGER_ATOM(TypedObject), OCLASP(TypedObjectModule)},
> +    {js_InitTypedObjectInt32x4Class, EAGER_ATOM(int32x4), &js::X4Type::class_},
> +    {js_InitTypedObjectFloat32x4Class, EAGER_ATOM(float32x4), &js::X4Type::class_},

add #ifdef

@@ -1257,4 @@
>      {js_InitObjectClass,        EAGER_ATOM(lookupGetter), &JSObject::class_},
>      {js_InitObjectClass,        EAGER_ATOM(lookupSetter), &JSObject::class_},
>  #endif
> -

No need to remove this blank line

::: js/src/jsprototypes.h
@@ +95,4 @@
>  IF_INTL(real,imaginary) (Intl,                  37,     js_InitIntlClass,          CLASP(Intl)) \
>  IF_BDATA(real,imaginary)(TypedObject,           38,     js_InitTypedObjectModuleObject,   OCLASP(TypedObjectModule)) \
>      imaginary(GeneratorFunction,     39,     js_InitIteratorClasses, dummy) \
> +IF_BDATA(real,imaginary)(SIMD,                  40,     js_InitSIMDClass, OCLASP(SIMD)) \

add #ifdef

::: js/src/tests/ecma_6/TypedObject/simd/float32x4abs.js
@@ +2,5 @@
> +var BUGNUMBER = 946042;
> +
> +var summary = 'float32x4 abs';
> +
> +var S = SIMD;

No need to abbreviate SIMD further: it's already pretty short ;)

@@ +8,5 @@
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- write some clever tests that ensure we have correct
> +  // float32 semantics in the borderline cases.

Don't forget about this

@@ +10,5 @@
> +
> +  // FIXME -- write some clever tests that ensure we have correct
> +  // float32 semantics in the borderline cases.
> +
> +  var a = float32x4(-1, 2, -3, 4);

This and all the other {int,float}32x4 accesses have to be done on the TypedObject global

@@ +11,5 @@
> +  // FIXME -- write some clever tests that ensure we have correct
> +  // float32 semantics in the borderline cases.
> +
> +  var a = float32x4(-1, 2, -3, 4);
> +  var c = S.float32x4.abs(a);

b (but really, I'd prefer something like "val" and "absVal" for the names. This being a test, it's slightly less important, though)

@@ +19,5 @@
> +  assertEq(c.w, 4);
> +
> +  if (typeof reportCompare === "function")
> +    reportCompare(true, true);
> +  print("Tests complete");

Not needed

@@ +24,5 @@
> +}
> +
> +test();
> +
> +

Only one blank line at EOF

::: js/src/tests/ecma_6/TypedObject/simd/float32x4getters.js
@@ +37,4 @@
>      g.call(v)
>    }, TypeError, "Getter applicable to structs");
>    assertThrowsInstanceOf(function() {
> +    var t = new int32x4(1, 2, 3, 4);

No, please revert this

::: js/src/vm/GlobalObject.h
@@ +35,5 @@
> +extern JSObject *
> +js_InitTypedObjectInt32x4Class(JSContext *cx, js::HandleObject obj);
> +
> +extern JSObject *
> +js_InitSIMDClass(JSContext *cx, js::HandleObject obj);

Please move all of these into the correct header files, and into the js namespace. For the last, that means moving it into SIMD.h, and renaming it to "InitSIMDClass" (we don't do the js_ prefix anymore: it's a left-over form when SpiderMonkey was C-only). Please move all the typed object initialization functions (including js_InitTypedObjectModuleObject) into TypedObject.h, and rename them, too.

	Niko Matsakis [:nmatsakis]
	Comment 16 • 11 years ago

Comment on attachment 8342451 [details] [diff] [review]
Bug946042.diff

Marking this patch obsolete as it is subsumed by later work.

	Niko Matsakis [:nmatsakis]
	Comment 17 • 11 years ago

Comment on attachment 8344314 [details] [diff] [review]
bug946042-diff.txt

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

till's comments are good as always, just added a few minor things.

::: js/src/builtin/SIMD.js
@@ +7,5 @@
> +  if (!ObjectIsTypedDatum(val))
> +    return false;
> +  var typeRepr = DATUM_TYPE_REPR(val);
> +  REPR_KIND(typeRepr);
> +  if (REPR_KIND(typeRepr) != JS_TYPEREPR_X4_KIND)

In response to till: The check of REPR_KIND is not redundant; you must check the KIND before you know if it is legal to check REPR_TYPE, as REPR_TYPE is only valid for certain kinds of TypeRepresentations (iirc anyhow).

@@ +86,5 @@
> +  var otf = Int32x4Or(tr,fr);
> +  return Int32x4BitsToFloat32x4(otf);
> +}
> +
> +#define DECLARE_SIMD_SPLAT(foo32, Foo32, FOO32, OPNAME) \

Per till's suggestion, perhaps "type32/Type32/TYPE32" would be better than "foo". :) (I know I put "foo" initially...)

@@ +254,5 @@
> +DECLARE_SIMD_SPLAT(float32,Float32, FLOAT32, Splat)
> +DECLARE_SIMD_OP_1ARG(float32, Float32, FLOAT32, Abs, ABS)
> +DECLARE_SIMD_OP_1ARG(float32, Float32, FLOAT32, Neg, NEG)
> +DECLARE_SIMD_OP_1ARG(float32, Float32, FLOAT32, Reciprocal, RECIPROCAL)
> +DECLARE_SIMD_OP_1ARG(float32, Float32, FLOAT32, ReciprocalSqrt, RECIPROCALSQRT)

sqrt is certainly the operation I am most concerned about with respect to float32/float64. Ideally we'd have tests for the border cases.

::: js/src/builtin/TypedObject.cpp
@@ +1203,4 @@
>      static const JSFunctionSpec TypeDescriptorMethods[];
>      static const JSPropertySpec TypedObjectProperties[];
>      static const JSFunctionSpec TypedObjectMethods[];
> +    static const JSFunctionSpec TypedObjectStaticMethods[];

In other cases, we called similar arrays `TypeObjectProperties` (vs `TypedObjectProperties`). Can you change this to match?

@@ +1211,4 @@
>      static const JSFunctionSpec TypeDescriptorMethods[];
>      static const JSPropertySpec TypedObjectProperties[];
>      static const JSFunctionSpec TypedObjectMethods[];
> +    static const JSFunctionSpec TypedObjectStaticMethods[];

And here too.

@@ +1649,5 @@
> +
> +    // if the global TypedObject doesn't exist, initialize it
> +    RootedId id(cx, NameToId(cx->names().TypedObject));
> +    bool hasP;
> +    if (!JSObject::hasProperty(cx, global, id, &hasP) && !hasP)

This is not the right way to check whether `TypedObject` exists. You should use `classIsInitialized`: 

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/GlobalObject.h?from=classisinitialized#189

See e.g. http://dxr.mozilla.org/mozilla-central/source/js/src/vm/GlobalObject.h?from=arrayclassinitialized#238

::: js/src/jsapi.cpp
@@ +1232,5 @@
>  #if JS_HAS_UNEVAL
>      {js_InitStringClass,        EAGER_ATOM(uneval), OCLASP(String)},
>  #endif
> +    {js_InitSIMDClass,               EAGER_ATOM(SIMD), OCLASP(SIMD)},
> +    {js_InitTypedObjectModuleObject,   EAGER_ATOM(TypedObject), OCLASP(TypedObjectModule)},

These entries should be behind an #ifdef for ENABLE_BINARYDATA

::: js/src/tests/ecma_6/TypedObject/simd/float32x4lessthan.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- write some clever tests that ensure we have correct

Can you remove these FIXMEs and either write such tests or else open a new bug?

	Niko Matsakis [:nmatsakis]
	Comment 18 • 11 years ago

Regarding the ifdefs: since this code depends on typed objects, it might be best to just reuse the ENABLE_BINARY_DATA compile flag. If we want to release SIMD before binary data as a whole, we'll have to adjust the flag so that the SIMD names are added to the global object but not the TypedObject module itself.

	Ivan Jibaja Assignee
	Comment 19 • 11 years ago

Most requested edits have been applied. 

Edits requesting build flags were not added yet as we will wait to see how/if we separate the SIMD and TypedObject implementations. In most places I've re-used the ENABLE_BINARYDATA flag for #ifdefs

Also created new bug (948379) to amend the tests for the correctness of border cases.

	Till Schneidereit [:till]
	Comment 20 • 11 years ago

Comment on attachment 8345250 [details] [diff] [review]
bug946042-diff.txt

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

This looks very good! There's quite a bit of additional feedback, but most of it is nits. Please still go through and address it thoroughly. r=me with that done and a successful try run.

::: js/src/builtin/SIMD.cpp
@@ +7,5 @@
> +/*
> + * JS SIMD package.
> + * Specification matches polyfill:
> + * https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd_tests.js
> + * The objects (float32x4, int32x4) are implemented in TypedObjects.cpp 

Nit: trailing whitespace, and missing period

@@ +101,5 @@
> +JSObject *
> +SIMDObject::initClass(JSContext *cx, Handle<GlobalObject *> global)
> +{
> +    // Create SIMD Object.
> +    RootedObject SIMD(cx, NewObjectWithGivenProto(cx, &SIMDObject::class_, nullptr,

This needs Object.prototype as its proto. See comment below for how

@@ +107,5 @@
> +    if (!SIMD)
> +        return nullptr;
> +
> +    // Create Float32x4 and Int32x4 object.
> +    RootedObject objProto(cx, global->getOrCreateObjectPrototype(cx));

Please move this above the SIMD object creation and use it as the proto for that

@@ +112,5 @@
> +    if (!objProto)
> +        return nullptr;
> +    RootedObject float32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, objProto, 
> +                           global, SingletonObject));
> +    RootedObject int32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, objProto, 

Nit: trailing whitespace here and above

@@ +114,5 @@
> +    RootedObject float32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, objProto, 
> +                           global, SingletonObject));
> +    RootedObject int32x4(cx, NewObjectWithGivenProto(cx, &JSObject::class_, objProto, 
> +                         global, SingletonObject));
> +    if(!float32x4 || !int32x4)

Split these up, so you test for success of float32x4 creation before attempting int32x4 creation

@@ +125,5 @@
> +        return nullptr;
> +    }
> +
> +    // Install Float32x4 & Int32x4 object as a properties of SIMD object.
> +    if (!JS_DefineProperty(cx, SIMD, "float32x4", OBJECT_TO_JSVAL(float32x4),

I didn't see this during my last review, but please use if JSObject::defineProperty for these two property definitions. They should look exactly like the SIMD property definition below.
(The reason is that if you're using a string literal, it has to be converted to a jsstring and atomized)

@@ +133,5 @@
> +    {
> +        return nullptr;
> +    }
> +
> +    // Everything is setup, install SIMD on the global object.

s/setup/set up/

@@ +135,5 @@
> +    }
> +
> +    // Everything is setup, install SIMD on the global object.
> +    RootedValue SIMDValue(cx, ObjectValue(*SIMD));
> +    global->setConstructor(JSProto_SIMD, SIMDValue);

Move this up by another two lines (and add a newline after it): otherwise the comment about everything being set up is still false

@@ +145,5 @@
> +
> +    return SIMD;
> +}
> +
> +JS_FRIEND_API(JSObject *)

I don't think this needs to be JS_FRIEND_API

::: js/src/builtin/SIMD.h
@@ +13,5 @@
> +#include "jsobj.h"
> +
> +/*
> + * JS SIMD functions.
> + * Spec matching polyfill:

"Spec-matching"

@@ +27,5 @@
> +
> +    static JSObject* initClass(JSContext *cx, Handle<GlobalObject *> global);
> +
> +    static bool toString(JSContext *cx, unsigned int argc, jsval *vp);
> +

nit: remove these two blank lines

@@ +30,5 @@
> +    static bool toString(JSContext *cx, unsigned int argc, jsval *vp);
> +
> +
> +};
> +    

nit: whitespace

@@ +33,5 @@
> +};
> +    
> +}  /* namespace js */
> +
> +extern JS_FRIEND_API(JSObject *)

I don't think this needs to be either extern or JS_FRIEND_API. However, I see that you specifically changed it to this, so maybe there's some compiler that needs this?

::: js/src/builtin/SIMD.js
@@ +77,5 @@
> +
> +function Int32x4Select(int32x4val, trueValue, falseValue) {
> +  if (!IsVectorDatum(int32x4val, JS_X4TYPEREPR_INT32) ||
> +       !IsVectorDatum(trueValue, JS_X4TYPEREPR_FLOAT32) ||
> +       !IsVectorDatum(falseValue, JS_X4TYPEREPR_FLOAT32)) 

nit: trailing whitespace

@@ +219,5 @@
> +  var lane2 = Load_##type32(x4val1, ((mask >> 4) & 0x3)<<2); \
> +  var lane3 = Load_##type32(x4val1, ((mask >> 6) & 0x3)<<2); \
> +  var T = GetTypedObjectModule(); \
> +  return T.type32##x4(lane0, lane1, lane2, lane3); \
> +}

I'd still like it much better if these #defines (from line 94 up to here) where in the TypedObjectSelfHosted.h header. Sorry for not being very clear about which #defines I wanted to see moved there in my last review

::: js/src/builtin/TypedObject.cpp
@@ +1456,4 @@
>      return true;
>  }
>  
> +JS_FRIEND_API(JSObject *)

Again, I don't know why the JS_FRIEND_API is needed. Please remove if it isn't, or explain why if it is

@@ +1501,4 @@
>      if (!float32x4Object)
>          return nullptr;
>  
> +    // install float32x4 as a property of the TypedObject object and

Nit: "Install"

@@ +1501,5 @@
>      if (!float32x4Object)
>          return nullptr;
>  
> +    // install float32x4 as a property of the TypedObject object and
> +    // of the global object

Nit: missing period

@@ +1523,5 @@
>      if (!int32x4Object)
>          return nullptr;
>  
> +    // install int32x4 as a property of the TypedObject object and
> +    // of the global object

Same two nits as above

::: js/src/builtin/TypedObject.h
@@ +576,5 @@
>  
>  } // namespace js
>  
> +extern JS_FRIEND_API(JSObject *)
> +js_InitTypedObjectModuleObject(JSContext *cx, HandleObject obj);

This is already declared in line 98. Removing it here still lets everything compile as expected for me

::: js/src/builtin/Utilities.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "TypedObjectConstants.h"
> +#include "TypedObjectSelfHosted.h"

Please move these to the bottom of the file, and add a comment above along the lines of
/* Macros for Typed Objects and SIMD. */

::: js/src/jsapi.cpp
@@ +1235,5 @@
>      {js_InitStringClass,        EAGER_ATOM(uneval), OCLASP(String)},
>  #endif
> +#ifdef ENABLE_BINARYDATA
> +    {js_InitSIMDClass,               EAGER_ATOM(SIMD), OCLASP(SIMD)},
> +    {js_InitTypedObjectModuleObject,   EAGER_ATOM(TypedObject), OCLASP(TypedObjectModule)},

nit: fix alignment of EAGER_ATOM

	Ivan Jibaja Assignee
	Comment 21 • 11 years ago

Because of NaN canonicalization (Bug 945382), properties and functions of the float32x4 and int32x4 objects have been reimplemented in C++ (instead of self-hosted).

Also, moved the float32x4 and int32x4 ctors to the SIMD module (as agreed in our last meeting).

All requested edits from last review have been applied

	Till Schneidereit [:till]
	Comment 22 • 11 years ago

Comment on attachment 8346217 [details] [diff] [review]
bug946042-diff.patch

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

Almost there. Some changes that were made for self-hosting SIMD weren't fully reverted, and there are nits left to address. Mostly, things look good, though. The main thing that bothers me is that I still don't fully understand why the *32x4 ctors have to exist on both the typedObject and SIMD globals. Please explain (in a comment in the code), why this is needed.

All the C-style casts in SIMD.cpp should be replaced by static_cast<foo>() C++-style ones.

A request: this review has been somewhat painful because of the mostly-unrelated refactorings compared to the previous version. For the next patch, please don't include any of these. They should be done in patches of their own.

::: js/src/builtin/SIMD.cpp
@@ +8,5 @@
> + * JS SIMD package.
> + * Specification matches polyfill:
> + * https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd_tests.js
> + * The objects float32x4 and int32x4 are implemented in TypedObjects.cpp.
> + * The SIMD functions are declared here and implemented in self-hosted SIMD.js.

this isn't true anymore

@@ +44,5 @@
> +        nullptr,                 /* checkAccess */
> +        nullptr,                 /* call        */
> +        nullptr,                 /* hasInstance */
> +        nullptr,                 /* construct   */
> +        nullptr

nit: these fields are all indended 4 spaces too much

@@ +64,5 @@
> +                                                  global, SingletonObject));
> +    if (!SIMD)
> +        return nullptr;
> +
> +    // Get Float32x4 and Int32x4 Objects from TypedObject

nit: missing period

@@ +74,5 @@
> +    {
> +        return nullptr;
> +    }
> +    RootedObject float32x4(cx);
> +    float32x4 = ToObject(cx, float32x4Value);

these two lines can just be
RootedObject float32x4(cx, int32x4Value.toObjectOrNull())

@@ +85,5 @@
> +    {
> +        return nullptr;
> +    }
> +    RootedObject int32x4(cx);
> +    int32x4 = ToObject(cx, int32x4Value);

same as above

@@ +87,5 @@
> +    }
> +    RootedObject int32x4(cx);
> +    int32x4 = ToObject(cx, int32x4Value);
> +    if(!int32x4)
> +        return nullptr;

Oh, but you don't need all this: you have stored the objects in slots on the global, after all. Hence, you can replace lines 68 to 91 with:
RootedObject int32x4(cx, global->int32x4TypeObject());
JS_ASSERT(int32x4);
RootedObject float32x4(cx, global->float32x4TypeObject());
JS_ASSERT(float32x4);

(the asserts aren't strictly required, but I think they still make sense)

@@ +90,5 @@
> +    if(!int32x4)
> +        return nullptr;
> +
> +
> +    // Install them as properties of SIMD object

nit: missing period

@@ +103,5 @@
> +    }
> +
> +    // Define Float32x4 & Int32x4 functions.
> +    if (!JS_DefineFunctions(cx, int32x4, Int32x4Methods) ||
> +        !JS_DefineFunctions(cx, float32x4, Float32x4Methods))

I'm not too fond of this split of initialization: the classes are created in TypedObject.cpp, but the methods installed here. If the ctors only lived on SIMD, all of this would go away ...

@@ +143,5 @@
> +    static Elem toType(Elem a) {
> +        return a;
> +    }
> +};
> +} // namespace js

no need to close and immediately re-open the js namespace

@@ +156,5 @@
> +        return obj.getInt32x4TypeObject();
> +    }
> +    static Elem toType(Elem a) {
> +        return ToInt32(a);
> +    }

please make the whitespace in this struct the same as in Float32x4 above

@@ +190,5 @@
> +
> +namespace js {
> +template<typename T, typename V>
> +struct Abs {
> +    static inline T apply(T x, T zero) {return V::toType(x < 0 ? (-1*x) : x);}

nit: `(-1*x)` should be `-1 * x` (i.e., less braces, more spaces)

@@ +316,5 @@
> +}
> +
> +template<typename V, typename Op, typename Vret>
> +static bool
> +Oper(JSContext *cx, unsigned argc, Value *vp)

not a fan of "Oper" at all. It's non-standard, and could stand for all of "Operation", "Operator" or "Operand".
Either rename to "Operation", or to "Op" (in which case you have to rename the template arg "Op" to something else, probably "Operation"). This applies to this function and all the "Oper*" ones below

@@ +359,5 @@
> +
> +        args.rval().setObject(*obj);
> +        return true;
> +    } else {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,JSMSG_TYPED_ARRAY_BAD_ARGS);

nit: missing space between arguments

@@ +412,5 @@
> +
> +        typename V::Elem *val = (typename V::Elem*) AsTypedDatum(args[0].toObject()).typedMem();
> +        typename Vret::Elem result[Vret::lanes];
> +        for (int32_t i = 0; i < Vret::lanes; i++){
> +            result[i] = val[OpShuffle::apply(i*2, args[1].toNumber())];

nit: missing whitespace around *

@@ +431,5 @@
> +        }
> +        typename V::Elem *val1 = (typename V::Elem*) AsTypedDatum(args[0].toObject()).typedMem();
> +        typename V::Elem *val2 = (typename V::Elem*) AsTypedDatum(args[1].toObject()).typedMem();
> +        typename Vret::Elem result[Vret::lanes];
> +        for (int32_t i = 0; i < Vret::lanes; i++){

nit: missing whitespace before {

@@ +432,5 @@
> +        typename V::Elem *val1 = (typename V::Elem*) AsTypedDatum(args[0].toObject()).typedMem();
> +        typename V::Elem *val2 = (typename V::Elem*) AsTypedDatum(args[1].toObject()).typedMem();
> +        typename Vret::Elem result[Vret::lanes];
> +        for (int32_t i = 0; i < Vret::lanes; i++){
> +            if(i<Vret::lanes/2)

nit: missing whitespace around < and /

@@ +433,5 @@
> +        typename V::Elem *val2 = (typename V::Elem*) AsTypedDatum(args[1].toObject()).typedMem();
> +        typename Vret::Elem result[Vret::lanes];
> +        for (int32_t i = 0; i < Vret::lanes; i++){
> +            if(i<Vret::lanes/2)
> +                result[i] = val1[OpShuffle::apply(i*2, args[2].toNumber())];

nit: missing whitespace around *

@@ +435,5 @@
> +        for (int32_t i = 0; i < Vret::lanes; i++){
> +            if(i<Vret::lanes/2)
> +                result[i] = val1[OpShuffle::apply(i*2, args[2].toNumber())];
> +            else
> +                result[i] = val2[OpShuffle::apply(i*2, args[2].toNumber())];

nit: missing whitespace around *

@@ +444,5 @@
> +
> +        args.rval().setObject(*obj);
> +        return true;
> +    } else {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,JSMSG_TYPED_ARRAY_BAD_ARGS);

nit: missing whitespace between arguments

@@ +464,5 @@
> +    }
> +    typename V::Elem *val = (typename V::Elem*) AsTypedDatum(args[0].toObject()).typedMem();
> +    typename Vret::Elem result[Vret::lanes];
> +    for (int32_t i = 0; i < Vret::lanes; i++)
> +        result[i] = (typename Vret::Elem)(val[i]);

nit: missing space after cast, and superfluous braces around "val[i]" (but this should be a C++-style cast, anyway)

@@ +554,5 @@
> +        return false;
> +    }
> +    int32_t result[Int32x4::lanes];
> +    for (int32_t i = 0; i < Int32x4::lanes; i++)
> +        result[i] =  args[i].toBoolean() ? 0xFFFFFFFF : 0x0;

nit: one space too many after the =

@@ +615,5 @@
> +    int32_t *val = (int32_t *) AsTypedDatum(args[0].toObject()).typedMem();
> +    int32_t *tv = (int32_t *) AsTypedDatum(args[1].toObject()).typedMem();
> +    int32_t *fv = (int32_t *) AsTypedDatum(args[2].toObject()).typedMem();
> +    int32_t tr[Int32x4::lanes];
> +    // var tr = SIMD.int32x4.and(t, tv);

remove commented js here and three times below

@@ +638,5 @@
> +    return true;
> +}
> +
> +const JSFunctionSpec js::Float32x4Methods[] = {
> +        JS_FN("abs", (Oper< Float32x4, Abs< float, Float32x4 >, Float32x4 >), 1, 0),

remove space after < and before > here and in all entries until EOF

::: js/src/builtin/SIMD.h
@@ +26,5 @@
> +    static const Class class_;
> +    static JSObject* initClass(JSContext *cx, Handle<GlobalObject *> global);
> +    static bool toString(JSContext *cx, unsigned int argc, jsval *vp);
> +};
> +    

nit: whitespace

@@ +29,5 @@
> +};
> +    
> +}  /* namespace js */
> +
> +extern JSObject *

the "extern" shouldn't be needed

::: js/src/builtin/TypedObject.cpp
@@ +1175,5 @@
> +    bool Type32##x4Lane##lane(JSContext *cx, unsigned argc, Value *vp) { \
> +       CallArgs args = CallArgsFromVp(argc, vp); \
> +       if(!IsTypedDatum(args.thisv().toObject())){ \
> +           JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO, \
> +                                X4Type::class_.name, "lane" + lane, InformalValueTypeName(args.thisv())); \

nit: put InformalValueTypeName on the next line

@@ +1197,5 @@
> +    bool Type32##x4SignMask(JSContext *cx, unsigned argc, Value *vp) { \
> +        CallArgs args = CallArgsFromVp(argc, vp); \
> +        if(!IsTypedDatum(args.thisv().toObject())){ \
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO, \
> +                                 X4Type::class_.name, "signMask", InformalValueTypeName(args.thisv())); \

same here

@@ +1379,4 @@
>        {                                                                       \
>          _type *mem = (_type*) TypedMem(*result);                              \
>          for (uint32_t i = 0; i < LANES; i++) {                                \
> +            mem[i] = ConvertScalar<_type>(values[i]);                          \

nit: move \ one column left

@@ -1445,5 @@
> -     * The initialization strategy for TypedObjects is mildly unusual
> -     * compared to other classes. Because all of the types are members
> -     * of a single global, `TypedObject`, we basically make the
> -     * initialized for the `TypedObject` class populate the
> -     * `TypedObject` global (which is referred to as "module" herein).

this comment shouldn't just be removed. Please move it into GlobalObject::initTypedObjectModule

@@ +1584,5 @@
>  
>      return module;
>  }
>  
> +JS_FRIEND_API(JSObject *)

this shouldn't need to be JS_FRIEND_API

::: js/src/builtin/TypedObject.h
@@ +592,4 @@
>  
>  } // namespace js
>  
> +extern JSObject *

this shouldn't need to be extern

::: js/src/builtin/TypedObject.js
@@ +39,5 @@
>  
> +// Handy shortcut
> +
> +#define DATUM_TYPE_REPR(obj) \
> +	 TYPE_TYPE_REPR(DATUM_TYPE_OBJ(obj))

The changes in this file all seem to be redundant now. AFAICT, you can just revert the file to its original state, right? Please still add the license header, though.

::: js/src/builtin/TypedObjectSelfHosted.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This entire file doesn't seem to be needed, or used, anymore. Please remove it.

::: js/src/vm/GlobalObject.h
@@ +419,5 @@
> +        JS_ASSERT(getSlotRef(FLOAT32X4_TYPE_OBJECT).isUndefined());
> +        setSlot(FLOAT32X4_TYPE_OBJECT, ObjectValue(obj));
> +    }
> +
> +    JSObject &getFloat32x4TypeObject() {

this should be const. Also, the naming convention for infallible getters is to leave off the "get", so it'd be "float32x4TypeObject"

@@ +420,5 @@
> +        setSlot(FLOAT32X4_TYPE_OBJECT, ObjectValue(obj));
> +    }
> +
> +    JSObject &getFloat32x4TypeObject() {
> +        // only gets called from contexts where known to be initialized

the comment isn't needed: this is a common pattern

@@ +430,5 @@
> +        JS_ASSERT(getSlotRef(INT32X4_TYPE_OBJECT).isUndefined());
> +        setSlot(INT32X4_TYPE_OBJECT, ObjectValue(obj));
> +    }
> +
> +    JSObject &getInt32x4TypeObject() {

same comments from above apply to this function

	Niko Matsakis [:nmatsakis]
	Comment 23 • 11 years ago

Till -- actually I think we can fix the float32x4 constructors to only live in SIMD now. I had to do some refactoring to make that possible, but I did it already (and it appears as part of that patch). (In particular, it is possible to create the TypeRepresentations without having the TypedObject module be created.)

Probably we can just move the relevant code over into SIMD and remove the line in SIMD that ensures that the TypedObject module is instantiated.

	Niko Matsakis [:nmatsakis]
	Comment 24 • 11 years ago

Actually, I might have been somewhat mistaken about the need to do any refactoring at all, now that I think about it. We could probably revert most of the changes to TypedObject.cpp that I made. I don't know if this actually *matters* -- not sure which is cleaner. Probably better to revert those parts of the patch though just on the premise "Less change, good". Also I think the older initialization style was a bit closer to what we see elsewhere (where the object is created using a method on global, but filled in with properties and so on by the jsprototype mechanism).

	Niko Matsakis [:nmatsakis]
	Comment 25 • 11 years ago

OK, I pushed a change to the branch that makes float32x4 avail only in the SIMD module. It turned out that the refactoring I had done *was* necessary, so most of the changes to TypedObject were still needed.

	Ivan Jibaja Assignee
	Comment 26 • 11 years ago

Edits applied.

The 32x4 objects now only live in the SIMD module.

	Ivan Jibaja Assignee
	Comment 27 • 11 years ago

Changed patch to 8 lines of context instead of 4

	Till Schneidereit [:till]
	Comment 28 • 11 years ago

Comment on attachment 8348317 [details] [diff] [review]
bug946042-diff.patch

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

Very nice! r=me with the remaining comments addressed

::: js/src/builtin/SIMD.cpp
@@ +7,5 @@
> +/*
> + * JS SIMD package.
> + * Specification matches polyfill:
> + * https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd_tests.js
> + * The objects float32x4 and int32x4 are built, implemented, and installed on the SIMD module.

"are installed on the SIMD pseudo-module". I don't think you need to specifically mention that they're implemented in this file, and I don't know what "built" means in this context.

@@ +38,5 @@
> +    static const int32_t lanes = 4;
> +    static const X4TypeRepresentation::Type type =
> +        X4TypeRepresentation::TYPE_FLOAT32;
> +
> +    static JSObject &GetTypeObject(GlobalObject &obj) {

nit: this should be called "global". Same in Int32x4.

@@ +73,5 @@
> +LaneAccessor(JSContext *cx, unsigned argc, Value *vp) {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if(!args.thisv().isObject() || !IsTypedDatum(args.thisv().toObject())) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO,
> +                             X4Type::class_.name, "lane" + lane,

You can't concat char buffers like that. Leftover from when this was a macro, I know, but watch out for the warnings the compiler gives you.

I think the easiest thing to do is to add this as the first or second line of LaneAccessors:
static const char *laneNames[] = {"lane 0", "lane 1", "lane 2", "lane3"};

and then change the `"lane" + lane` to `laneNames[lane]` here and below

@@ +80,5 @@
> +    }
> +    TypedDatum &datum = AsTypedDatum(args.thisv().toObject());
> +    TypeRepresentation *typeRepr = datum.datumTypeRepresentation();
> +    if (typeRepr->kind() != TypeRepresentation::X4 ||
> +        typeRepr->asX4()->type() != L::type)

nit: fits in 100 columns

@@ +88,5 @@
> +                             InformalValueTypeName(args.thisv()));
> +        return false;
> +    }
> +    typename L::Elem *data =
> +        reinterpret_cast<typename L::Elem *>(datum.typedMem());

nit: fits in 100 columns

@@ +90,5 @@
> +    }
> +    typename L::Elem *data =
> +        reinterpret_cast<typename L::Elem *>(datum.typedMem());
> +    L::setReturn(args, data[lane]);
> +        return true;

nit: indentation is off

@@ +97,5 @@
> +template<typename L>
> +static bool
> +SignMask(JSContext *cx, unsigned argc, Value *vp) {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if(!args.thisv().isObject() || !IsTypedDatum(args.thisv().toObject())){

nit: missing whitespace before {

@@ +103,5 @@
> +                             X4Type::class_.name, "signMask", InformalValueTypeName(args.thisv()));
> +        return false;
> +    }
> +    typename L::Elem *data =
> +        reinterpret_cast<typename L::Elem *>(AsTypedDatum(args.thisv().toObject()).typedMem());

I think it wouldn't hurt to split this up as in LaneAccessor above:
TypedDatum &datum = AsTypedDatum(args.thisv().toObject());
and then call typedMem on that

@@ +191,5 @@
> +
> +const JSFunctionSpec js::Float32x4Defn::TypeObjectMethods[] = {
> +    JS_SELF_HOSTED_FN("toSource", "X4ToSource", 0, 0),
> +    JS_FS_END
> +};

please move the float things above the int things here, to keep consistency with everywhere else

@@ +314,5 @@
> +JSObject *
> +SIMDObject::initClass(JSContext *cx, Handle<GlobalObject *> global)
> +{
> +    // Create SIMD Object.
> +    RootedObject objProto(cx, global->getOrCreateObjectPrototype(cx));

objProto needs a null check

@@ +339,5 @@
> +    }
> +
> +    // int32x4
> +
> +    RootedObject int32x4Object(cx, CreateX4Class<Int32x4Defn>(cx,global));

nit: whitespace missing after cx,

@@ +364,5 @@
> +    {
> +        return nullptr;
> +    }
> +
> +    return SIMD;

this whole function got a lot cleaner, nice

@@ +687,5 @@
> +    typename V::Elem *val =
> +        reinterpret_cast<typename V::Elem *>(AsTypedDatum(args[0].toObject()).typedMem());
> +    typename Vret::Elem result[Vret::lanes];
> +    for (int32_t i = 0; i < Vret::lanes; i++)
> +        result[i] = (typename Vret::Elem) val[i];

I'm guessing this is the cast you mentioned yesterday? ;)

::: js/src/builtin/SIMD.h
@@ +6,5 @@
> +
> +#ifndef builtin_SIMD_h
> +#define builtin_SIMD_h
> +
> +#include "mozilla/MemoryReporting.h"

what's this import needed for? If it's not needed, please remove. If it's only needed in the implementation, move it to the cpp file

@@ +14,5 @@
> +
> +/*
> + * JS SIMD functions.
> + * Spec matching polyfill:
> + * https://github.com/johnmccutchan/ecmascript_simd/blob/master/src/ecmascript_simd_tests.js

this should refer to the implementation, not the tests

::: js/src/builtin/TypedObject.cpp
@@ +1212,5 @@
>  
> +/*  The initialization strategy for TypedObjects is mildly unusual
> + * compared to other classes. Because all of the types are members
> + * of a single global, `TypedObject`, we basically make the
> + * initialized for the `TypedObject` class populate the

nit: "initializer"

@@ +1347,5 @@
>                                                    TypeRepresentation::Kind kind,
>                                                    const Class **clasp,
>                                                    MutableHandleObject proto)
>  {
> +    Rooted<GlobalObject *> global(cx, cx->global());

please add an assert for global here. It shouldn't ever fail, but I'm not sure enough of this to not assert it

@@ +2635,5 @@
> +bool
> +js::GetFloat32x4TypeObject(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Rooted<GlobalObject*> global(cx, cx->global());

same assert for global here and in the int version below

::: js/src/builtin/TypedObject.h
@@ +546,5 @@
>  
>  /*
> + * Usage: GetFloat32x4TypeObject()
> + *
> + * Returns the float32x4 type object. SIMD module must have been

"The SIMD pseudo-module" here and for the int32 equivalent below

	Ivan Jibaja Assignee
	Comment 29 • 11 years ago

r=till
checkin-needed

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

Comment on attachment 8348924 [details] [diff] [review]
bug946042-diff.patch

This doesn't apply cleanly. Please rebase. Also, you can just use the checkin-needed bug keyword when you're ready. Thanks!

	Ivan Jibaja Assignee
	Comment 31 • 11 years ago

rebased. checkin-needed

	Till Schneidereit [:till]
	Comment 32 • 11 years ago

Comment on attachment 8349011 [details] [diff] [review]
bug946042.patch

Something went very wrong with this rebase: builtin/SIMD.cpp is missing now.

@ijibaja, please fix, make sure the shell builds, and attach a new version. For that patch, don't set the checkin flag: niko will push to try before landing, and the checkin flag can cause sheriffs to land the patch without try. (This would've happened if the previous patch had applied cleanly.)

	Ivan Jibaja Assignee
	Comment 33 • 11 years ago

Ready to be pushed to try.

	Till Schneidereit [:till]
	Comment 34 • 11 years ago

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=268631eb0b28

	Till Schneidereit [:till]
	Comment 35 • 11 years ago

And another try run: https://tbpl.mozilla.org/?tree=Try&rev=b663e0b314a0

I over-eagerly cancelled the previous one because I thought that style checker errors stopped important parts of some tasks. Turns out they don't.

	Till Schneidereit [:till]
	Comment 36 • 11 years ago

So, GCC 4.4 doesn't like how you're using templates when installing accessors: https://hg.mozilla.org/try/file/008cf80871b0/js/src/builtin/SIMD.cpp#l160 (and following).

I have no idea how to work around that, but it shouldn't be too hard. Otherwise, the try run looks good: the two oranges are intermittend failures.

	Niko Matsakis [:nmatsakis]
	Comment 37 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/abdeb55e8a16

Try run: https://tbpl.mozilla.org/?tree=Try&rev=308637dba332

	Carsten Book [:Tomcat]
	Comment 38 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/abdeb55e8a16

	Andrew McCreight [:mccr8]
	Comment 39 • 11 years ago

Changes to ecmaGlobals in test_interfaces.html require review from a JS peer.  Though Niko landed it, so I guess that sort of counts. ;)

	Florian Scholz (Open Web Docs)
	Comment 40 • 9 years ago

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SIMD and sub pages.