1031203 - SIMD: Implement float64x2 in the interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Float64x2 have been added to the straw man proposal in [1]. This shouldn't be too hard with all the macros we already have. I will do it unless somebody beats me to it; it seems like a nice "good next bug".

[1] https://github.com/johnmccutchan/ecmascript_simd/commit/8941215c41fa0e93988768feb9be4799390724da

Comment 1

[:ProgramFOX]

To do this, is it necessary to create a new X2TypeDescr class, or can float64x2 belong to the X4TypeDescr class?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to ProgramFOX from comment #1)
> To do this, is it necessary to create a new X2TypeDescr class, or can
> float64x2 belong to the X4TypeDescr class?

I forgot to express that in the bug: I think the intent was to rename X4TypeDescr into SimdTypeDescr and reuse it. It sounds like it would be a good first patch for this bug, if you're interested in working on it.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Hey, any news about that? Feel free to post a WIP patch, so that we know somebody's working on it :) Otherwise I might take it on next week or so.

Comment 4

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Hey, any news about that? Feel free to post a WIP patch, so that we know
> somebody's working on it :) Otherwise I might take it on next week or so.

Sorry for the late reply, I was on vacation and didn't have Internet access there.

I have been working on this bug for a while, but I got stuck at a point when the new float64x2 test cases gave an error about arguments (not when creating a new float64x2, but when performing an operation on it). I will upload my WIP patch and the error that I get in a new attachment/comment (I cannot do it right now, I'm currently building the source and then I'll run the tests again to show the exact error.

Comment 5

[:ProgramFOX]

Attachment: Work In Progress patch

This is what I currently have. I get these errors when running the test cases:

> REFTEST TEST-UNEXPECTED-FAIL | file:///home/programfox/mozilla-source/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/test-stage/jsreftest/tests/jsreftest.html?test=ecma_6/TypedObject/simd/float64x2abs.js | Unknown file:///home/programfox/mozilla-source/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/test-stage/jsreftest/tests/ecma_6/TypedObject/simd/float64x2abs.js:13: Error: invalid arguments item 1
Line 13 looks like this:
> var c = SIMD.float64x2.abs(a);
This happens for many test cases, not only this one, and the error always occurs on the line where the operation gets performed (abs, add, sqrt ...)

I also get an error on the converting methods:
> REFTEST TEST-UNEXPECTED-FAIL | file:///home/programfox/mozilla-source/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/test-stage/jsreftest/tests/jsreftest.html?test=ecma_6/TypedObject/simd/float64x2getters.js | Unknown file:///home/programfox/mozilla-source/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/test-stage/jsreftest/tests/ecma_6/TypedObject/simd/float64x2getters.js:17: TypeError: X4.prototype.lane 0 called on incompatible TypedObject item 1
(That's not the only one where I get the error on).

And I also get an error on the 'float64x2 alignment', because I got 32, but expected 16 (well, I didn't really expect this, I just copied this from float32x4 because I wasn't sure what should be *really* expected).

Comment 6

[:ProgramFOX]

Attachment: bug-1031203-work-in-progress2.diff

Same comment as above, I only fixed a minor error.

Comment 7

[:ProgramFOX]

Attachment: bug-1031203-work-in-progress3.diff

Apparently I didn't fix that minor mistake; fixed it now.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8459654 [details] [diff] [review]
bug-1031203-work-in-progress3.diff

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

That looks really good! Thanks a lot for doing that. I see that you also included the X4 -> Simd renaming, which is really nice but makes the patch look a little bit big. How about splitting up this patch into several parts? I'd suggest the following split:
- X4 -> Simd renaming
- maybe some cleanups in SIMD.h (I see you've moved a few function declarations).
- Implement Float64x2
- JS ref tests.

If you're using Mercurial, you can use the Crecord extension [0], as described in [1]. Otherwise, git has the |add -p| feature which is equivalent, but somehow less practical.

Once again, that's already really good. Don't feel discouraged by my comments. This is a big task and it's normal to receive a lot of feedback about it. Once you've fixed the few non-nits, you should retry to run tests and see if things get better. Don't hesitate to ask questions here or in #jsapi if you need to :)

[0] http://mercurial.selenic.com/wiki/CrecordExtension
[1] https://blog.mozilla.org/bjacob/2011/03/02/some-mercurial-queues-tips-hg-qcrecord-qfold-and-qpush-move/

::: js/src/builtin/SIMD.cpp
@@ +79,5 @@
>      FOUR_LANES_ACCESSOR(Int32x4);
>      FOUR_LANES_ACCESSOR(Float32x4);
>  #undef FOUR_LANES_ACCESSOR
> +
> +#define TWO_LANES_ACCESSOR(type) \

No need to define TWO_LANES_ACCESSOR, as it will be only used once for the moment (there are no plans for other x2 values as far as I know).

@@ +124,5 @@
>  static bool type##SignMask(JSContext *cx, unsigned argc, Value *vp) { \
>      return SignMask<Int32x4>(cx, argc, vp); \
>  }
>      SIGN_MASK(Float32x4);
> +    SIGN_MASK(Float64x2);

SignMask<T> needs some refactoring to take into account that there are only two lanes here.

@@ +238,2 @@
>                Handle<GlobalObject*> global,
>                HandlePropertyName stringRepr)

nit: keep alignment of following arguments please

@@ +293,3 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    unsigned LANES = 4;

LANES should depend on the SimdTypeDescr.

@@ +300,4 @@
>          return false;
>      }
> +    else {
> +        LANES = args.length();

Correct me if I am wrong, but one could create an int32x4 with only two arguments, with this change, I think.

@@ +377,1 @@
>                                                     cx->names().float32x4);

nit: keep arguments aligned, here and in the rest of the patch please :)

::: js/src/builtin/SIMD.h
@@ +194,5 @@
>  
> +struct Float64x2 {
> +    typedef double Elem;
> +    static const unsigned lanes = 2;
> +    static const SimdTypeDescr::Type type = SimdTypeDescr::TYPE_FLOAT32;

typo here: should be TYPE_FLOAT64.

::: js/src/builtin/TypedObject.cpp
@@ +434,1 @@
>      sizeof(_type) * 4,

This doesn't work for Float64, as you expect to have 2 Float64, not 4. You might need to add a fourth parameter to JS_FOR_EACH_SIMD_TYPE_REPR, which is the number of lanes (4 for int32 and float32, 2 for float64).

::: js/src/builtin/TypedObject.js
@@ +480,5 @@
>    return NewDerivedTypedObject(newArrayType, this, 0);
>  }
>  
>  ///////////////////////////////////////////////////////////////////////////
>  // X4

Could you rename that into SIMD as well, please?

@@ +491,3 @@
>      return "float32x4";
> +  case JS_SIMDTYPEREPR_FLOAT64:
> +    return "float64X2";

nit: X shouldn't be a capital:

  return "float64x2";

@@ +508,3 @@
>  
>    var type = DESCR_TYPE(descr);
> +  

nit: trailing whitespace here

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromfloat64x2bits.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

For Float64, the only (interesting) border cases will be for +/- Infinity, NaN, +/-0. Feel free to add more tests per file or to let it like this and add a comment in bug 948379.

::: js/src/vm/GlobalObject.h.rej
@@ +8,5 @@
> +     static const unsigned INTRINSICS              = DEBUGGERS + 1;
> +     static const unsigned FLOAT32X4_TYPE_DESCR   = INTRINSICS + 1;
> +     static const unsigned INT32X4_TYPE_DESCR     = FLOAT32X4_TYPE_DESCR + 1;
> +     static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
> ++    static const unsigned FLOAT64X2_TYPE_DESCR    = FOR_OF_PIC_CHAIN + 1;

This file shouldn't be in your patch.

Comment 9

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

Thanks for the feedback! This is the first part of the new patches, in which I renamed X4 to SIMD.

Comment 10

[:ProgramFOX]

Attachment: Cleanups in SIMD.h

Second part of the patch; some cleanups in SIMD.h (re-sorting the methods, I broke this order when working on bug 1031198).

Comment 11

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

First part of patch re-uploaded, I had too much spaces at a point, that's fixed now.

Comment 12

[:ProgramFOX]

I just noticed that I didn't fix all whitespace issues in that patch. Will fix them in the third part.

Comment 13

[:ProgramFOX]

Attachment: Part 3 (implement float64x2): Work In Progress

This is the Work In Progess part 3 of the patch. It already runs test cases where input and output type have the same amout of lanes fine, but it fails at those where the output type has a different amount of lanes (such as equal). I don't know the cause of this; I edited CoercedFunc to handle this, but the problem is still there. Currently, it only handles it on functions with two arguments (such as equal), but it also fails on those methods.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8460868 [details] [diff] [review]
Renamed X4 to SIMD

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

Nice work, thanks! Can you please the last few remarks and upload a new patch (don't forget to edit the commit message to add r=bbouvier)

::: js/src/builtin/SIMD.cpp
@@ +37,4 @@
>  
>  static const char *laneNames[] = {"lane 0", "lane 1", "lane 2", "lane3"};
>  
> +template<typename TypeSimd, int lane>

Type32x4 had a sense as Type was directly referring to Int or Float, but now it feels awkward to read TypeSimd rather than SimdType. If you agree, can you name the first template parameter SimdType, please?

@@ +198,5 @@
>  };
>  
>  template<typename T>
>  static JSObject *
> +CreateSimdClass(JSContext *cx,

while you're here, can you fix the alignment here, please? (it's better when all related changes are in a single patch)

@@ +339,1 @@
>                                                     cx->names().float32x4);

alignment here

@@ +356,1 @@
>                                                 cx->names().int32x4);

alignment here

::: js/src/builtin/TypedObject.js
@@ +501,3 @@
>  
>    var type = DESCR_TYPE(descr);
> +    return SimdProtoString(type)+"("+this.x+", "+this.y+", "+this.z+", "+this.w+")";

nit: return lost alignment here

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8460852 [details] [diff] [review]
Cleanups in SIMD.h

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

Nice. Can you change the commit message into: Bug 1031203: Reordered declarations in SIMD.h; r=bbouvier

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8461642 [details] [diff] [review]
Part 3 (implement float64x2): Work In Progress

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

Thanks for splitting it! That would definitely help if you could also split the tests into a separate patch (all tests files).
Looks pretty good overall. See my comment in CoercedFunc which should fix your issue.

::: js/src/builtin/SIMD.cpp
@@ +79,5 @@
>      FOUR_LANES_ACCESSOR(Int32x4);
>      FOUR_LANES_ACCESSOR(Float32x4);
>  #undef FOUR_LANES_ACCESSOR
> +
> +    LANE_ACCESSOR(Float64x2, 0); 

nit: trailing whitespace

@@ +113,5 @@
> +    if (TypeSimd::type != SimdTypeDescr::Type::TYPE_FLOAT64) {
> +        int32_t mz = data[2] < 0.0 ? 1 : 0;
> +        int32_t mw = data[3] < 0.0 ? 1 : 0;
> +        result = mx | my << 1 | mz << 2 | mw << 3;
> +    } 

nit: trailing whitespace

@@ +116,5 @@
> +        result = mx | my << 1 | mz << 2 | mw << 3;
> +    } 
> +    else {
> +         result = mx | my << 1;
> +    }

We can make it more generic with a loop and thus delete the if/else (this is pseudo code):

result = data[0] < 0.0 ? 1 : 0;
for (i = 1; i < number of lanes; i++)
  result |= (data[i] < 0.0 ? 1 : 0) << i;

@@ +237,5 @@
>  template<typename T>
>  static JSObject *
>  CreateSimdClass(JSContext *cx,
> +                Handle<GlobalObject*> global,
> +                HandlePropertyName stringRepr)

This should belong to your first patch :)

@@ +293,5 @@
>  bool
>  SimdTypeDescr::call(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    const unsigned LANES = args.callee().as<SimdTypeDescr>().type() == SimdTypeDescr::Type::TYPE_FLOAT64 ? 2 : 4;

Hmm, having these if/else makes it more complicated to read. How about:
1) adding a slot to SimdTypeDescr class (change JSCLASS_HAS_RESERVED_SLOTS(JS_DESCR_SLOTS) to JSCLASS_HAS_RESERVED_SLOTS(JS_SIMD_DESCR_SLOTS) where JS_SIMD_DESCR_SLOTS == JS_DESCR_SLOTS + 1).
2) storing the number of lanes in this last slot, in CreateSimdClass
3) reuse it here

@@ +298,2 @@
>  
> +    if (args.length() < 2) {

nit: args.length() < LANES

@@ +300,2 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +                             args.callee().getClass()->name, "1", "s");

This should be conditional on the class: 1 for Float64x2, 3 for the others.

@@ +319,4 @@
>        case _constant:                                                         \
>        {                                                                       \
>          _type *mem = reinterpret_cast<_type*>(result->typedMem());            \
>          for (unsigned i = 0; i < LANES; i++)                                  \

you can use _lanes rather than LANES here

@@ +389,5 @@
>  
> +    // float64x2
> +    RootedObject float64x2Object(cx);
> +    float64x2Object = CreateSimdClass<Float64x2Defn>(cx, global,
> +                                                   cx->names().float64x2);

nit: alignment

@@ +666,5 @@
> +        if (Coercion::lanes == In::lanes) {
> +            for (unsigned i = 0; i < Coercion::lanes; i++)
> +                result[i] = Op::apply(left[i], right[i]);
> +        }
> +        else {

nit: } else {

@@ +672,5 @@
> +            for (unsigned i = 0; i < Coercion::lanes; i++, j += In::lanes / Coercion::lanes) {
> +                int x = floor(j);
> +                result[i] = Op::apply(left[x], right[x]);
> +            }
> +        }

I wouldn't put an if/else, as the second branch (more generic) is supposed to work in all cases.

The algorithm is busted: because of integer division, In::lanes (== 2) / Coercion::lanes (== 4) will be 0 always, so j = 0 and x = 0 all way long. You have to explicit the multiplication:

for (unsigned i = 0; i < Coercion::lanes; i++) {
  unsigned j = (i * In::lanes) / Coercion::lanes;
  result[i] = Op::apply(left[j], right[j]);
}

(no way to hoist In::lanes / Coercion::lanes, as it would be 0 all the time -- parenthesis matter here).

@@ +950,5 @@
>      return true;
>  }
>  
>  static bool
> +Float64x2Clamp(JSContext *cx, unsigned argc, Value *vp)

You should be able to reuse Float32x4 by making it more generic.

::: js/src/builtin/SIMD.h
@@ +24,5 @@
>  #define FLOAT32X4_UNARY_FUNCTION_LIST(V)                                                                          \
>    V(abs, (Func<Float32x4, Abs<float, Float32x4>, Float32x4>), 1, 0, Abs)                                          \
>    V(fromInt32x4, (FuncConvert<Int32x4, Float32x4> ), 1, 0, FromInt32x4)                                           \
>    V(fromInt32x4Bits, (FuncConvertBits<Int32x4, Float32x4>), 1, 0, FromInt32x4Bits)                                \
> +  V(fromFloat64x2, (FuncConvert<Float64x2, Float32x4>), 1, 0, FromFloat64x2)                                      \

If you're okay with waiting on bug 1042244 to land, declarations in this file should be way shorter (see patch 5 of the mentioned bug). However, this will also imply that if you wait for these to land, you'll have to rebase your patch, which is painful but part of the multiplayer developer process :). I entirely let you the choice here, let me know what you prefer.

::: js/src/builtin/TypedObject.h
@@ +367,5 @@
>  
>  #define JS_FOR_EACH_SIMD_TYPE_REPR(macro_)                             \
> +    macro_(SimdTypeDescr::TYPE_INT32, int32_t, int32, 4)               \
> +    macro_(SimdTypeDescr::TYPE_FLOAT32, float, float32, 4)             \
> +    macro_(SimdTypeDescr::TYPE_FLOAT64, double, float64, 2)

Nice

::: js/src/builtin/TypedObject.js
@@ +505,5 @@
>  
>    if (DESCR_KIND(descr) != JS_TYPEREPR_SIMD_KIND)
>      ThrowError(JSMSG_INCOMPATIBLE_PROTO, "SIMD", "toSource", typeof this);
>  
> +  var type = DESCR_TYPE(descr); 

nit: trailing whitespace

@@ +512,1 @@
>      return SimdProtoString(type)+"("+this.x+", "+this.y+", "+this.z+", "+this.w+")";

Could you rather make the second part the output of a new function, let's say 'SimdValuesString(type, this)' ?

::: js/src/builtin/TypedObjectConstants.h
@@ -56,5 @@
>  #define JS_TYPEREPR_SCALAR_KIND         1
>  #define JS_TYPEREPR_REFERENCE_KIND      2
>  #define JS_TYPEREPR_STRUCT_KIND         3
>  #define JS_TYPEREPR_SIZED_ARRAY_KIND    4
> -#define JS_TYPEREPR_SIMD_KIND             5

hmm, this should be in the first patch as well.

Comment 17

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

Fixed last remarks.

Comment 18

[:ProgramFOX]

Attachment: Reordered declarations in SIMD.h

Comment 19

[:ProgramFOX]

> If you're okay with waiting on bug 1042244 to land, declarations in this
> file should be way shorter (see patch 5 of the mentioned bug). However, this
> will also imply that if you wait for these to land, you'll have to rebase
> your patch, which is painful but part of the multiplayer developer process
> :). I entirely let you the choice here, let me know what you prefer.
I'll wait for these changes, and then add the declarations. If I wouldn't wait, then sooner or later, the declarations have to be changed, so then it's quite useless to add something that will be changed in a short time.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8462469 [details] [diff] [review]
Renamed X4 to SIMD

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

Looks good, thanks!
For next times, as I already r+'d your patch, you didn't need to ask again for review, you can just set it as r+ by yourself as you did for the other patch.

Comment 21

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> For next times, as I already r+'d your patch, you didn't need to ask again
> for review, you can just set it as r+ by yourself as you did for the other
> patch.

Good to know, thanks for telling me!

Comment 22

[:ProgramFOX]

Attachment: Implement float64x2: Work In Progress

A new Work In Progress patch. I get some build errors. I think they are caused in TypedObject.h, but I've looked at it and cannot find the cause.

This is the error:
> /home/programfox/mozilla-source/mozilla-central/js/src/builtin/TypedObject.h:345:7: note: ‘js::SimdTypeDescr::SimdTypeDescr(const js::SimdTypeDescr&)’ is implicitly deleted because the default definition would be ill-formed:
>  class SimdTypeDescr : public ComplexTypeDescr
This happens for all types in TypedObject.h. And these errors cause others, like:
> /home/programfox/mozilla-source/mozilla-central/js/src/builtin/TypedObject.h:345:7: error: use of deleted function ‘js::ComplexTypeDescr::ComplexTypeDescr(const js::ComplexTypeDescr&)’

Comment 23

[:ProgramFOX]

I forgot to say: I didn't yet fix all remarks, but I think the build errors have nothing to do with the remarks that are not yet fixed.

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8462959 [details] [diff] [review]
Implement float64x2: Work In Progress

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

See below.

::: js/src/builtin/SIMD.cpp
@@ +287,5 @@
>  bool
>  SimdTypeDescr::call(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    SimdTypeDescr typeDescr = args.callee().as<SimdTypeDescr>();

The first error is shown at this line. SimdTypeDescr as all JSObjects cannot be copy-constructed, so typeDescr needs to be a reference. (In this context, it's unused so you could even just get rid of it; don't take this comment into account if you plan to use it :))

Comment 25

[:ProgramFOX]

Attachment: float64x2-work-in-progress-coercedfunc.diff

In this Work In Progress patch, I merged the two Clamp methods into one, added border cases to some of the testcases and tried to fix the CoercedFunc method. However, I'm still getting the same error in the test case, even after fixing the loop. Example error (in float64x2equal.js):
> float64x2equal.js:15:2 Error: Assertion failed: got -1, expected 0
Line 15 looks like this:
>assertEq(e.x, 0);
And I'm comparing 1 and 10, so that should return 0.

Comment 26

[:ProgramFOX]

I also tried to use Op<InElem>::apply instead of Op<CoercionElem>::apply, but that gave a lot of compiler errors.

Comment 27

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8467028 [details] [diff] [review]
float64x2-work-in-progress-coercedfunc.diff

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

I can't apply your patch cleanly, because it's apparently based upon another patch you made. Can you rebase or upload a rolled up patch, please?

Also, please, please split this patch into two parts (one with the implementation and one with the tests). That will really help reviewing.

Regarding your issues, I'd suggest using a debugger and step into CoerceFunc to see what goes wrong. You could also make sure that the values inserted in the float64x2 are correct (does the float64x2getters test pass?). After some thinking, comparison operators should all be CoerceFunc<inputType, inputType, op, int32x4>, which makes me even think that Func should actually call Func<in, in, op, out>. Does any of those two solutions work?

::: js/src/builtin/SIMD.cpp
@@ +108,5 @@
>  
>      MOZ_ASSERT(!typedObj.owner().isNeutered());
>      Elem *data = reinterpret_cast<Elem *>(typedObj.typedMem());
> +    int32_t result = data[0] < 0.0 ? 1 : 0;
> +    for (unsigned i = 1; i < SimdTypeDescr::lanes(descr.as<SimdTypeDescr>().type()); i++) {

can you hoist above the loop the value of descr.as<SimdTypeDescr>().type()

@@ +633,5 @@
>  CoercedFunc(JSContext *cx, unsigned argc, Value *vp)
>  {
>      typedef typename Coercion::Elem CoercionElem;
>      typedef typename Out::Elem RetElem;
> +    typedef typename In::Elem InElem;

nit: unused

::: js/src/vm/GlobalObject.h.rej
@@ +1,1 @@
> +--- GlobalObject.h

This file shouldn't be in the patch.

Comment 28

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #27)
> Comment on attachment 8467028 [details] [diff] [review]
> float64x2-work-in-progress-coercedfunc.diff
> 
> I can't apply your patch cleanly, because it's apparently based upon another
> patch you made. Can you rebase or upload a rolled up patch, please?
That's strange, it applied fine for me without I had problems. I rebased it after bug 1042244 got fixed, so that might cause the problem.

I'll try your suggestions, thanks a lot!

Comment 29

[:ProgramFOX]

Attachment: Part 3: Implement float64x2: Work In Progress

New Work In Progress patch. The equality operations test cases run fine now. Now I'm having problems at these test cases:

    ecma_6/TypedObject/simd/float64x2setter.js
    ecma_6/TypedObject/simd/int32x4fromfloat64x2bits.js
    ecma_6/TypedObject/simd/float64x2reify.js
    ecma_6/TypedObject/simd/float64x2fromint32x4bits.js

At setter and reify, I get an error when getting a value from the array:
> Error: No such property on self-hosted object: typedObject
float64x2fromint32x4bits gives a wrong result and int32x4fromfloat64x2bits says "Invalid arguments". I'm not sure how the FuncConvertBits method works, so I couldn't fix that.

Comment 30

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8468423 [details] [diff] [review]
Part 3: Implement float64x2: Work In Progress

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

This seems to go to the right direction. Interestingly, I have made the same kind of changes in my local patch queue, which really needs to get r? and land to make things even easier for you. Sorry for blocking your work like this, but hopefully this should make things simpler and help tests passing.

Comment 31

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

Because of changes in SIMD.cpp and SIMD.h (Func has been split into UnaryFunc and BinaryFunc), I had to rebase my patches. This is the rebased patch 1 (X4 to SIMD).

Comment 32

[:ProgramFOX]

Attachment: Reordered declarations in SIMD.h

Patch 2 (reordered declarations in SIMD.h) rebased.

Comment 33

[:ProgramFOX]

Attachment: Implemented float64x2: work in progress

This is the rebased part 3 of the patch (implemented float64x2, work in progress). When running the tests, I still get the same errors.

Comment 34

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] (away 08/15 to 08/25) from comment #30)
> Comment on attachment 8468423 [details] [diff] [review]
> Part 3: Implement float64x2: Work In Progress
> 
> Review of attachment 8468423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to go to the right direction. Interestingly, I have made the same
> kind of changes in my local patch queue, which really needs to get r? and
> land to make things even easier for you. Sorry for blocking your work like
> this, but hopefully this should make things simpler and help tests passing.

That's interesting. I agree that it needs an r?, that would make things easier.

Comment 35

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8475963 [details] [diff] [review]
Renamed X4 to SIMD

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

Sorry for the long review! Looks good to me. Will need rebasing, and then I'll land it for you, to avoid it rotting again (no need to ask again for r?).

::: js/src/builtin/SIMD.cpp
@@ +230,5 @@
>          return nullptr;
>  
>      // Create type constructor itself and initialize its reserved slots.
>  
> +    Rooted<SimdTypeDescr*> simd(cx);

could you name it typeDescr instead, please?

Comment 36

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8475966 [details] [diff] [review]
Reordered declarations in SIMD.h

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

No need to ask again for r+ next time, as this a pretty trivial change ;) Thanks anyways!

Comment 37

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

Rebased patch with the changes from comment 35 applied.

Comment 38

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

I had to rebase the patch again, so here is the new one. After it gets r+'ed, I'll push to try and add checkin-needed (if the try push returns green) to make sure that I don't have to rebase it again.

The reordering of the declarations patch is still up-to-date, so I don't need to rebase that again now.

Comment 39

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8489144 [details] [diff] [review]
Renamed X4 to SIMD

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

Thanks for the rebasing and sorry about the last issue, but I don't think CreateX4 should be renamed.
When you have fixed this last issue, please submit another patch for review, and in parallel you can push it to the try server, to spare one round trip.

::: js/src/jit/IonTypes.h
@@ +273,5 @@
>    public:
>      // Doesn't have a default constructor, as it would prevent it from being
>      // included in unions.
>  
> +    static SimdConstant CreateSimd(int32_t x, int32_t y, int32_t z, int32_t w) {

Here and below, the name CreateX4 should be kept, as it gives some useful hint about the number of arguments (this is especially useful for the form that receives an array). Can you please let it as is?

Comment 40

[:ProgramFOX]

Attachment: Renamed X4 to SIMD

New patch with CreateX4 kept.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=7292c5e3aaf5

Comment 41

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8489468 [details] [diff] [review]
Renamed X4 to SIMD

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

Thanks a lot, let's land this!

Comment 42

[Benjamin Bouvier [:bbouvier] (inactive)]

Landing the two first parts, to help:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/106da91bbf79
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/46b607594969

Thanks again for your work :)

Comment 43

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8476060 [details] [diff] [review]
Implemented float64x2: work in progress

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

Sorry if I holded back this review that long. I actually feel this approach is consistent with the rest of the file right now, so that would be fine to have. Let me know if you don't want to work on it anymore cause you don't have time or anything, in which case I'll take care of completing it and will let your name in the authors list :)

uber-nit: no more JS_ASSERT() please, only MOZ_ASSERT in the newer patches.

::: js/src/builtin/SIMD.cpp
@@ +127,5 @@
>  
>      MOZ_ASSERT(!typedObj.owner().isNeutered());
>      Elem *data = reinterpret_cast<Elem *>(typedObj.typedMem());
> +    int32_t result = data[0] < 0.0 ? 1 : 0;
> +    for (unsigned i = 1; i < SimdTypeDescr::lanes(descr.as<SimdTypeDescr>().type()); i++) {

please hoist the lanes values above the loop + can you just start from 0 (assigning result to 0 at start)?

@@ +312,4 @@
>  
>      if (args.length() < LANES) {
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +                             args.callee().getClass()->name, LANES == 4 ? "3" : "1", "s");

please hoist the ternary above

@@ +333,4 @@
>        case _constant:                                                         \
>        {                                                                       \
>          _type *mem = reinterpret_cast<_type*>(result->typedMem());            \
> +        for (unsigned i = 0; i < _lanes; i++)                                 \

you can just reuse LANES, here; if the use case for adding an argument to STORE_LANES was this place, you can just delete the last macro arg

@@ +689,5 @@
>  }
>  
> +template<typename In, template<typename C> class Op>
> +static bool
> +FuncEq(JSContext *cx, unsigned argc, Value *vp)

please rename it CompareFunc or something more appropriate you can think of

@@ +692,5 @@
> +static bool
> +FuncEq(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    typedef typename In::Elem InElem;
> +    typedef Int32x4 Out;

no need for typedef'ing Int32x4 as Out, and Out::Elem as RetElem, etc. You can just use Int32x4 at the right places, int32_t as Out::Elem, etc.

@@ +699,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != 2)
> +        return ErrorBadArgs(cx);
> +
> +    RetElem result[Out::lanes];

please move this declaration right before the first use

@@ +700,5 @@
> +    if (args.length() != 2)
> +        return ErrorBadArgs(cx);
> +
> +    RetElem result[Out::lanes];
> +    if (!IsVectorObject<In>(args[0]) || !IsVectorObject<In>(args[1]))

group this check with the args.length() check please

@@ +710,5 @@
> +        unsigned j = (i * In::lanes) / Out::lanes;
> +        result[i] = Op<InElem>::apply(left[j], right[j]);
> +    }
> +
> +    RetElem *coercedResult = reinterpret_cast<RetElem *>(result);

Please use StoreResult here.

@@ +838,5 @@
>      if (args.length() != 1 || !IsVectorObject<V>(args[0]))
>          return ErrorBadArgs(cx);
>  
>      Elem *val = TypedObjectMemory<Elem *>(args[0]);
> +

nit: blank line

@@ +848,5 @@
> +
> +    if (V::lanes < Vret::lanes) {
> +        for (unsigned i = Vret::lanes - V::lanes; i < Vret::lanes; i++)
> +            result[i] = 0;
> +    }

you can make these few lines simpler with i < Vret::lanes, and having an if inside the body of the for loop

::: js/src/builtin/SIMD.h
@@ +208,5 @@
> +    static Elem toType(Elem a) {
> +        return a;
> +    }
> +    static bool toType(JSContext *cx, JS::HandleValue v, Elem *out) {
> +        *out = v.toNumber();

What if the object given as an arg actually isn't a number but an object, for instance?

::: js/src/builtin/TypedObject.js
@@ +505,5 @@
>  
>    if (DESCR_KIND(descr) != JS_TYPEREPR_SIMD_KIND)
>      ThrowError(JSMSG_INCOMPATIBLE_PROTO, "SIMD", "toSource", typeof this);
>  
> +  var type = DESCR_TYPE(descr); 

nit: trailing whitespace

@@ +507,5 @@
>      ThrowError(JSMSG_INCOMPATIBLE_PROTO, "SIMD", "toSource", typeof this);
>  
> +  var type = DESCR_TYPE(descr); 
> +  var protoString = SimdProtoString(type);
> +  if (protoString !== "float64x2")

instead, how about creating a function SimdTypeToLength, that would return 4 for int32x4 and float32x4, and 2 for float64x2 (a la SimdProtoString), and use it here in this condition?

::: js/src/builtin/TypedObjectConstants.h
@@ +45,5 @@
>  #define JS_DESCR_SLOT_STRUCT_FIELD_TYPES 7
>  #define JS_DESCR_SLOT_STRUCT_FIELD_OFFSETS 8
>  
> +// Slots on SimdTypeDescr
> +#define JS_DESCR_SLOT_LANES              9

Couldn't you reuse one of the previous slots?

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/106da91bbf79
https://hg.mozilla.org/mozilla-central/rev/46b607594969

Comment 45

[:ProgramFOX]

Attachment: Implemented Float64x2

I fixed the remarks in this new patch. I could fix the test case errors in the previous patch. They were caused by: a) an incorrect variable name in TypedObject.js. b) an incorrect variable name in a test case. c) some incorrect test data in a test case.

I'll upload the test cases in a different patch.

Comment 46

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #43)
> Comment on attachment 8476060 [details] [diff] [review]
> Implemented float64x2: work in progress

> you can just reuse LANES, here; if the use case for adding an argument to STORE_LANES was this place, you can just delete the last macro arg
I did not delete the last argument, because STORE_LANES uses JS_FOR_EACH_SIMD_TYPE_REPR, and this macro is also used in the SimdLanes struct (which needs the _lanes argument).
> What if the object given as an arg actually isn't a number but an object, for instance?
We've been discussing this on IRC, and you asked to look for a ToDouble(3 args) method like the ToInt32(3 args) method. There isn't a method like this, so I didn't change this.
> Couldn't you reuse one of the previous slots?
No, I have tried this and many tests are failing if I reuse another one.

Comment 47

[:ProgramFOX]

Attachment: Float64x2 test cases

This patch contains the Float64x2 test cases. They do not contain border cases yet, because I assumed that there is no standardized behavior yet.

Comment 48

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8492292 [details] [diff] [review]
Implemented Float64x2

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

Getting better and better, we're really close to land it! This looks great overall, there is just one or two non-nits I'd like to see addressed before r+.

Totally not mandatory, but if you can, could you split up the patch, such that the CompareFunc refactoring is in one patch, and adding the Float64x2 is in another? Once again, totally not mandatory.

::: js/src/builtin/SIMD.cpp
@@ +165,5 @@
>  }
>  
>  #define SIGN_MASK(type) \
>  static bool type##SignMask(JSContext *cx, unsigned argc, Value *vp) { \
>      return SignMask<Int32x4>(cx, argc, vp); \

pre-existing: while you're here, can you please change this line to

return SignMask<type>(cx, argc, vp);

@@ +356,2 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +                             args.callee().getClass()->name, min_lanes, "s");

actually, could you use (atoi(LANES) - 1) here? not sure that stdlib is available from this file, so if it's not, nevermind and keep the code like this :)

@@ +369,5 @@
>          return false;
>  
>      MOZ_ASSERT(!result->owner().isNeutered());
>      switch (descr->type()) {
> +#define STORE_LANES(_constant, _type, _name, _lanes)                          \

This will need rebasing.

@@ +748,5 @@
> +        unsigned j = (i * In::lanes) / Int32x4::lanes;
> +        result[i] = Op<InElem>::apply(left[j], right[j]);
> +    }
> +
> +    return StoreResult<Int32x4>(cx, args, result);

Nice!

@@ +872,5 @@
>  
>      Elem *val = TypedObjectMemory<Elem *>(args[0]);
>      RetElem result[Vret::lanes];
>      for (unsigned i = 0; i < Vret::lanes; i++)
> +        if (i < V::lanes)

please use a ternary here:

result[i] = i < V::lanes ? ConvertScalar<RetElem>(val[i]) : 0;

(otherwise, having an if statment on several lines would need braces for the "for" body, as our style guide needs)

@@ +947,5 @@
>          result[i] = args[i].toBoolean() ? 0xFFFFFFFF : 0x0;
>      return StoreResult<Int32x4>(cx, args, result);
>  }
>  
> +template<typename T>

Can you rename T into In, and TElem into InElem?

@@ +1006,2 @@
>  static bool
> +FloatSelect(JSContext *cx, unsigned argc, Value *vp)

I really think Float32x4Select and Int32x4Select could be factored out together, but I don't want to slow down this bug anymore. Feel free to open a follow up bug and fix it, if you're up to ;)

::: js/src/builtin/SIMD.h
@@ +99,5 @@
> +  V(scale, (FuncWith<Float64x2, Scale>), 2, 0)                                      \
> +  V(sub, (BinaryFunc<Float64x2, Sub, Float64x2>), 2, 0)                             \
> +  V(withX, (FuncWith<Float64x2, WithX>), 2, 0)                                      \
> +  V(withY, (FuncWith<Float64x2, WithY>), 2, 0)                                      \
> +  V(withZ, (FuncWith<Float64x2, WithZ>), 2, 0)                                      \

Float64x2 has only two lanes, so it shouldn't have the withY and withZ methods.

::: js/src/builtin/TypedObject.cpp
@@ +439,5 @@
> +    _lanes,
> +    JS_FOR_EACH_SIMD_TYPE_REPR(SIMD_LANE) 0
> +#undef SIMD_LANE
> +};
> +

nit: 2 blank lines here, please keep only one.

@@ +459,5 @@
> +{
> +    return SimdLanes[t];
> +}
> +
> +

nit: ditto

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

nit: MOZ_ASSERT in new code, JS_ASSERT is deprecated

::: js/src/builtin/TypedObject.js
@@ +497,5 @@
> +  return undefined;
> +}
> +
> +function SimdTypeToLength(protoString) {
> +  switch (protoString) {

please switch on type, not the string (it's more expensive to do so)

::: js/src/vm/GlobalObject.h
@@ +393,5 @@
>          return getSlotRef(FLOAT32X4_TYPE_DESCR).toObject();
>      }
>  
> +    void setFloat64x2TypeDescr(JSObject &obj) {
> +        JS_ASSERT(getSlotRef(FLOAT64X2_TYPE_DESCR).isUndefined());

nit: MOZ_ASSERT in new code, and same below

Comment 49

[:ProgramFOX]

Attachment: Implemented Float64x2: work in progress

I fixed the remarks (aside from the CompareFunc split), and when running the tests, I got an error in float64x2setter.js and float64x2reify.js:
> ecma_6/TypedObject/simd/float64x2setter.js:21:2 TypeError: SIMD requires more than 3 arguments
But all other tests run fine.

Comment 50

[:ProgramFOX]

Forgot to mention in my previous comment: if the test fail is fixed, I'll split up the patch as requested.

Comment 51

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to ProgramFOX from comment #49)
> Created attachment 8494653 [details] [diff] [review]
> Implemented Float64x2: work in progress
> 
> I fixed the remarks (aside from the CompareFunc split), and when running the
> tests, I got an error in float64x2setter.js and float64x2reify.js:
> > ecma_6/TypedObject/simd/float64x2setter.js:21:2 TypeError: SIMD requires more than 3 arguments
> But all other tests run fine.

Are you running tests with a debug build? (i.e. your ./configure line looked like ./configure --enable-debug etc.) If you're not sure about it, you can use a script that helps you defining configure lines, like this one [0] </advertisement>.

With your patches applied on trunk, I get into an assertion failure at start, because we try to set more slots than the maximum available number of slots in the TypedObject. I think you modified that in a previous patch but that got lost somehow.

The fix is to change this line in TypedObjectConstants.h:
-#define JS_DESCR_SLOTS                   9
+#define JS_DESCR_SLOTS                   10

Comment 52

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8494653 [details] [diff] [review]
Implemented Float64x2: work in progress

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

Just two remarks in SIMD.cpp btw.

::: js/src/builtin/SIMD.cpp
@@ +336,5 @@
>  bool
>  SimdTypeDescr::call(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    const unsigned LANES = SimdTypeDescr::lanes(args.callee().as<SimdTypeDescr>().type());

nit: please rename this one into 'lanes', as it isn't a static constant anymore.

@@ +351,5 @@
>          return true;
>      }
>  
>      if (args.length() < LANES) {
> +        const char* min_lanes = LANES == 4 ? "3" : "1";

How about having a switch here too (or in a helper function)?

const char*
SimdTypeToMinimumLanesNumber(SimdTypeDescr &descr) {
switch (descr.type()) {
  case SimdTypeDescr::TYPE_INT32:
  case SimdTypeDescr::TYPE_FLOAT32:
    return "3";
  case SimdTypeDescr::TYPE_FLOAT64:
    return "1";
}
MOZ_CRASH("unexpected simd type descr");
}

Comment 53

[Benjamin Bouvier [:bbouvier] (inactive)]

The CompareFunc refactoring is needed for avoiding differential testing issues, like in bug 1068725 where asm.js/no-asmjs yield different results without it.

Comment 54

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #51)
> Are you running tests with a debug build? (i.e. your ./configure line looked
> like ./configure --enable-debug etc.) If you're not sure about it, you can
> use a script that helps you defining configure lines, like this one [0]
> </advertisement>.

Link [0] is actually https://github.com/bnjbvr/.files/blob/master/spiderbuild.py

Comment 55

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8492308 [details] [diff] [review]
Float64x2 test cases

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

Great job! For most test cases (actually, all not involving comparisons and conversions from float/double to int32), you can implement border cases right now, as they don't depend upon the spec.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromfloat64x2.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

You can add border cases for this particular operator, as they shouldn't be an issue.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromfloat64x2bits.js
@@ +8,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

@@ +10,5 @@
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.
> +
> +  var a = float64x2.fromInt32x4Bits(int32x4(0x3F800000, 0x40000000, 0x40400000, 0x40800000));

you can simply use the float64x2 constructor here, otherwise you're testing fromInt32x4Bits and fromFloat64x2Bits altogether. Then you can remove the unused int32x4 import.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2abs.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

I think you can implement these here.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2add.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

ditto

::: js/src/tests/ecma_6/TypedObject/simd/float64x2alignment.js
@@ +5,5 @@
> +var summary = 'float64x2 alignment';
> +
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/

Please add this header to all the new test files, please.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2div.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

ditto

::: js/src/tests/ecma_6/TypedObject/simd/float64x2equal.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

ditto

@@ +13,5 @@
> +  var b = float64x2(10, 20);
> +  var e = float64x2.equal(a, b);
> +  assertEq(e.x, 0);
> +  assertEq(e.y, 0);
> +  assertEq(e.z, -1); // equal returns an int32x4, where x and y are the results for the first float64 and z and w for the second float64

can you put this comment above the first assertEq, and split it on several lines?

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromfloat32x4.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

All numbers that can be represented as float32 are expected to be exactly representable as float64, so you can already test border cases here.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromfloat32x4bits.js
@@ +8,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

ditto

@@ +11,5 @@
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.
> +
> +  var a = float32x4.fromInt32x4Bits(int32x4(0x00000000, 0x3ff00000, 0x0000000, 0x40000000));
> +  var c = float64x2.fromFloat32x4Bits(a);

no need to use an intermediate int32x4 here, otherwise you're testing both fromFloat32x4Bits and fromInt32x4Bits in the meanwhile here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromint32x4.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

All int32 can be represented precisely as float32, iirc, so please add border cases here as well.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromint32x4bits.js
@@ +7,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

you can remove this one

::: js/src/tests/ecma_6/TypedObject/simd/float64x2greaterthan.js
@@ +11,5 @@
> +
> +  var a = float64x2(1, 20);
> +  var b = float64x2(10, 2);
> +  var c = float64x2.greaterThan(b,a);
> +  assertEq(c.x, -1);

A comment as in float64x2equal would be appreciated :)

::: js/src/tests/ecma_6/TypedObject/simd/float64x2greaterthanorequal.js
@@ +14,5 @@
> +  var c = float64x2(30, 40);
> +  var d = float64x2(30, 4);
> +  var e = float64x2.greaterThanOrEqual(b, a);
> +  var f = float64x2.greaterThanOrEqual(d, c);
> +  

nit: trailing whitespace

+ Please add a comment here too

::: js/src/tests/ecma_6/TypedObject/simd/float64x2handle.js
@@ +27,5 @@
> +  // float64x2 fails.
> +
> +  assertThrowsInstanceOf(function() {
> +    var h = float64.handle(array, 1, "w");
> +  }, TypeError, "Creating a float32 handle to prop via ctor");

nit: in this string and next ones in this file, s/float32/float64

::: js/src/tests/ecma_6/TypedObject/simd/float64x2lessthan.js
@@ +10,5 @@
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.
> +
> +  var a = float64x2(1, 20);
> +  var b = float64x2(10, 2);
> +  var c = float64x2.lessThan(a, b);

please add a comment as in the equal test file

::: js/src/tests/ecma_6/TypedObject/simd/float64x2lessthanorequal.js
@@ +14,5 @@
> +  var c = float64x2(30, 40);
> +  var d = float64x2(30, 4);
> +  var e = float64x2.lessThanOrEqual(a, b);
> +  var f = float64x2.lessThanOrEqual(c, d);
> +  assertEq(e.x, -1);

ditto

::: js/src/tests/ecma_6/TypedObject/simd/float64x2mul.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

we can do it right now here as well

::: js/src/tests/ecma_6/TypedObject/simd/float64x2neg.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

ditto

::: js/src/tests/ecma_6/TypedObject/simd/float64x2notequal.js
@@ +10,5 @@
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.
> +
> +  var a = float64x2(1, 20);
> +  var b = float64x2(10, 20);
> +  var c = float64x2.notEqual(a, b);

please add a comment as in equal

::: js/src/tests/ecma_6/TypedObject/simd/float64x2reciprocal.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

we can do them right here right now

::: js/src/tests/ecma_6/TypedObject/simd/float64x2reciprocalsqrt.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2scale.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2shuffle.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2shufflemix.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2sqrt.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2sub.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

::: js/src/tests/ecma_6/TypedObject/simd/float64x2with.js
@@ +6,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 948379: Amend to check for correctness of border cases.

same here

@@ +11,5 @@
> +
> +  var a = float64x2(1, 2);
> +  var x = float64x2.withX(a, 5);
> +  var y = float64x2.withY(a, 5);
> +  assertEq(x.x, 5);

can you also assertEq(x.y, 2); and assertEq(y.x, 1)

::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat64x2bits.js
@@ +12,5 @@
> +
> +  var a = float64x2(1.0, 2.0);
> +  var c = int32x4.fromFloat64x2Bits(a);
> +  assertEq(c.x, 0x00000000);
> +  assertEq(c.y, 0x3FF00000);

This might behave weirdly with respect to the machine's endianness. Please make sure to try build on all our platforms to be sure this one passes well.

Comment 56

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #51)
> With your patches applied on trunk, I get into an assertion failure at
> start, because we try to set more slots than the maximum available number of
> slots in the TypedObject. I think you modified that in a previous patch but
> that got lost somehow.

Where exactly have you seen this assertion failure? I have used your script to get configuration with --enable-debug, but I cannot see that assertion failure. I also tried to change that line in TypedObjectConstants.h, but that didn't work.

Comment 57

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to ProgramFOX from comment #56)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #51)
> > With your patches applied on trunk, I get into an assertion failure at
> > start, because we try to set more slots than the maximum available number of
> > slots in the TypedObject. I think you modified that in a previous patch but
> > that got lost somehow.
> 
> Where exactly have you seen this assertion failure? I have used your script
> to get configuration with --enable-debug, but I cannot see that assertion
> failure. I also tried to change that line in TypedObjectConstants.h, but
> that didn't work.

Still get it when running the shell (i.e. launching ./dist/bin/js in the builddir):
Assertion failure: index < (((getClass())->flags >> 8) & (((uint32_t)1 << (8)) - 1)), /inbound/js/src/jsobj.h:459

Here are my configure flags:
/inbound/js/src/configure  --enable-gcgenerational --enable-exact-rooting --enable-debug --disable-optimize --without-intl-api

Please make sure to operate in a new build dir if you can't see it / disable ccache, etc.

Comment 58

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #57)
> 
> Still get it when running the shell (i.e. launching ./dist/bin/js in the
> builddir):
> Assertion failure: index < (((getClass())->flags >> 8) & (((uint32_t)1 <<
> (8)) - 1)), /inbound/js/src/jsobj.h:459
> 
> Here are my configure flags:
> /inbound/js/src/configure  --enable-gcgenerational --enable-exact-rooting
> --enable-debug --disable-optimize --without-intl-api
> 
> Please make sure to operate in a new build dir if you can't see it / disable
> ccache, etc.

Yes, those flags worked fine, and I get the failure when I set the number of slots to 9. When I set them to 10, I don't get the failure but the two test cases still fail. I'm going to try to find the reason.

Comment 59

[:ProgramFOX]

Attachment: Implemented Float64x2 test cases

I haven't yet found the reason for the two test fails, so I first updated the other test cases. The tests are currently failing at float64x2reify and float64x2setter (those were the two failing test cases), and float64x2abs (it gives -0 for abs(-0)), but abs fails because of the same reason as in bug 948379, so that one should land first before this bug.

Comment 60

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8504171 [details] [diff] [review]
Implemented Float64x2 test cases

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

Thanks for keeping on this patch. Unfortunately, there's not much I can do: the wip patch for float64x2 integration doesn't apply cleanly anymore on inbound, so i cannot help you debugging these tests. Plus, if you tell me they don't all pass, then i cannot set the r+ flag. Could we focus on the float64x2 patch first, and then the tests in a second place?

Comment 61

[:ProgramFOX]

I have been trying to do the debugging, but I really fail at it. I have taken a look at https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb but it does not really provide the information I need. How can I debug SIMD.cpp and TypedObject.js?

Comment 62

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to ProgramFOX from comment #61)
> I have been trying to do the debugging, but I really fail at it. I have
> taken a look at
> https://developer.mozilla.org/en-US/docs/Debugging_Mozilla_with_gdb but it
> does not really provide the information I need. How can I debug SIMD.cpp and
> TypedObject.js?

The TLDR is to compile only the shell in debug mode [0] and then run the shell inside gdb. Anybody should be able to help you in IRC on #developers or #jsapi, if you're getting stuck. Good luck!

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Developer_%28debug%29_build

Comment 63

[:ProgramFOX]

Attachment: Implemented Float64x2: work in progress

Work in progress patch, after rebasing.

I did the debugging and apparently, when I try to fetch a value from the array, it thinks I want a float32x4 so it complains that there aren't enough lane values given. I haven't yet found the cause, but I'll continue debugging.

Comment 64

[:ProgramFOX]

It took long, but I finally found the cause! I will upload the patches soon, because I still have to make some changes to keep it up-to-date with the polyfill, but I already wanted to inform you that they are coming soon.

The cause was just a stupid typo, and it took long to figure that out because the debugger wasn't helpful for that, I unforeseenly spotted it into my patch file.

The typo was (in SelfHosting.cpp):
> JS_FN("GetFloat64x2TypeDescr", js::GetFloat32x4TypeDescr, 0, 0),
>                                            ^^^^

Comment 65

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to ProgramFOX from comment #64)
> It took long, but I finally found the cause! I will upload the patches soon,
> because I still have to make some changes to keep it up-to-date with the
> polyfill, but I already wanted to inform you that they are coming soon.

Thanks for keeping on working on this bug! Let me know or don't hesitate to show up on #jsapi on IRC if you have any other issues.

Comment 66

[:ProgramFOX]

Attachment: Implemented Float64x2

Comment 67

[:ProgramFOX]

Attachment: Added float64x2 test cases

The patch containing the test cases.

Try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab563ccdeb1f

Comment 68

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #55)
> Comment on attachment 8492308 [details] [diff] [review]
> Float64x2 test cases
> 
> Review of attachment 8492308 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat64x2bits.js
> 
> This might behave weirdly with respect to the machine's endianness. Please
> make sure to try build on all our platforms to be sure this one passes well.

Seems like it passed, the try push (see comment 67) succeeded on all platforms. (It still shows "1 failed", but that's because I had to retrigger the B2G Desktop OS X opt job, now it passed fine)

Comment 69

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8529206 [details] [diff] [review]
Implemented Float64x2

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

Sorry for the slow review, previous week was a work week.

Thanks a lot for this patch, and great work! There are a few nits that I'd like to see addressed before r+ing. I'll review tests tomorrow. Don't forget to rebase your patches in the meanwhile ;)

::: js/src/builtin/SIMD.cpp
@@ +155,5 @@
>      Elem *data = reinterpret_cast<Elem *>(typedObj.typedMem());
> +    int32_t lanes = SimdTypeDescr::lanes(descr.as<SimdTypeDescr>().type());
> +    int32_t result = 0;
> +    for (int32_t i = 0; i < lanes; i++) {
> +      result |= (data[i] < 0.0 ? 1 : 0) << i;

nit: no {} around a for loop with only one single statement

@@ +475,5 @@
> +    {
> +        return nullptr;
> +    }
> +
> +

nit: please delete one of the two blank lines

@@ +1007,5 @@
>  
>      return StoreResult<Int32x4>(cx, args, orInt);
>  }
>  
> +template <typename T>

please rename T to In

@@ +1012,2 @@
>  static bool
> +FloatSelect(JSContext *cx, unsigned argc, Value *vp)

Not sure I've already asked you, but can FloatSelect and Int32x4Select be merged? I'd understand if you don't want to explore this right now, so that could be a follow up bug.

::: js/src/builtin/TypedObject.h
@@ +898,5 @@
>   */
>  bool GetFloat32x4TypeDescr(JSContext *cx, unsigned argc, Value *vp);
>  
>  /*
> + * Usage: GetFloat64x2TypeDescr()

can you add a line after this one, to ensure consistency between the different comments around?

::: js/src/builtin/TypedObjectConstants.h
@@ +43,5 @@
>  #define JS_DESCR_SLOT_STRUCT_FIELD_NAMES 7
>  #define JS_DESCR_SLOT_STRUCT_FIELD_TYPES 8
>  #define JS_DESCR_SLOT_STRUCT_FIELD_OFFSETS 9
>  
> +// Slots for SimdTypeDescr

This should say "Slots for SIMD objects". Also, above there is a comment refering to x4s, can you update it to "SIMD" please, while you're here?

@@ +44,5 @@
>  #define JS_DESCR_SLOT_STRUCT_FIELD_TYPES 8
>  #define JS_DESCR_SLOT_STRUCT_FIELD_OFFSETS 9
>  
> +// Slots for SimdTypeDescr
> +#define JS_DESCR_SLOT_LANES              10

Actually, can't this value be 8? The initialization in CreateSimdClass doesn't seem to use slots beyond JS_DESCR_SLOT_TYPE. Can you please try, and update this constant accordingly? (if it works, i think you can also let the former value of JS_DESCR_SLOTS below)

@@ +50,2 @@
>  // Maximum number of slots for any descriptor
> +#define JS_DESCR_SLOTS                   11

see above

Comment 70

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8529207 [details] [diff] [review]
Added float64x2 test cases

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

Thanks for adding all these tests! A few non-nits, so another review round will be necessary. Also, my long-term plan is to reduce the number of files, so you can keep on your logic of merging arithmetic operations tests into a single test file (as you've done for float64x2arithmetic.js -- you can also add abs / sqrt / reciprocal / reciprocalsqrt in this file if you want; otherwise this can be done in a follow-up bug). Good work!

::: js/src/tests/ecma_6/TypedObject/simd/comparisons.js
@@ +116,5 @@
> +          testNotEqualFloat64x2(v, w);
> +          testLessThanFloat64x2(v, w);
> +          testLessThanOrEqualFloat64x2(v, w);
> +          testGreaterThanFloat64x2(v, w);
> +          testGreaterThanOrEqualFloat64x2(v, w);   

nit: trailing whitespace

@@ +118,5 @@
> +          testLessThanOrEqualFloat64x2(v, w);
> +          testGreaterThanFloat64x2(v, w);
> +          testGreaterThanOrEqualFloat64x2(v, w);   
> +      }
> +  } 

ditto

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromfloat64x2.js
@@ +6,5 @@
> +var summary = 'float32x4 fromFloat64x2';
> +
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/

In this file and all others, the correct link is https://creativecommons.org/publicdomain/zero/1.0/

@@ +10,5 @@
> + * http://creativecommons.org/licenses/publicdomain/
> + */
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);

We need some cases where the float64 to float32 coercion is not correct. For instance, Math.fround(Math.pow(2, 25) - 1) can be represented exactly as a float64 but not as a float32. Please add a test case and if it fails to be consistent on all platforms, please comment it with a TODO.

::: js/src/tests/ecma_6/TypedObject/simd/float32x4fromfloat64x2bits.js
@@ +13,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  var a = float64x2(2.000000473111868, 512.0001225471497);

Here as well, I'd rather use the aliased typed array trick, but that's fine at the moment.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2-arithmetic.js
@@ +29,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  for ([v, w] of [[float64x2(1, 2), float64x2(3, 4)],

Please define v and w above this loop:

var v, w;

(i may have forgotten to do so in float32x4-arithmetic, so feel free to fix it if that's the case)

@@ +37,5 @@
> +  {
> +      testAdd(v, w);
> +      testSub(w, v);
> +      testMul(v, w);
> +      testDiv(w, v);

Any reason why you've interleaved operands this way? Could you add testSub(v, w) and testDiv(v, w) (as these are the two not-symmetric operations)

::: js/src/tests/ecma_6/TypedObject/simd/float64x2-minmax.js
@@ +1,2 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 946042;

Please remove any BUGNUMBER reference, add the public license link. Feel also free to merge this file and float32x4-minmax.js into a new file minmax.js

::: js/src/tests/ecma_6/TypedObject/simd/float64x2clamp.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 1031203;
> +var float64x2 = SIMD.float64x2;
> +
> +var summary = 'float64x2 clamp';

Feel free to merge with float32x4clamp.js into clamp.js

@@ +11,5 @@
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  // FIXME -- Bug 1068028: Amend to check for correctness of NaN/-0/Infinity/-Infinity border cases once the semantics are defined.

Please add test case for Infinity / -Infinity already.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromfloat32x4.js
@@ +26,5 @@
> +  var g = float32x4(Infinity, -Infinity, Infinity, -Infinity);
> +  var i = float64x2.fromFloat32x4(g);
> +  assertEq(i.x, Infinity);
> +  assertEq(i.y, -Infinity);
> +

Could you add a test with a non-exactly representable float32 value (e.g. 13.37) in the original float32x4 vector?

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromfloat32x4bits.js
@@ +21,5 @@
> +
> +  var d = float32x4(NaN, -0, Infinity, -Infinity);
> +  var f = float64x2.fromFloat32x4Bits(d);
> +  assertEq(f.x, -1.058925634e-314);
> +  assertEq(f.y, -1.404448428688076e+306);

I'd prefer to use the aliased typed array trick here, but that can be done in a follow up bug and I am fine with this at the moment.

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromint32x4.js
@@ +17,5 @@
> +  var c = float64x2.fromInt32x4(a);
> +  assertEq(c.x, 1);
> +  assertEq(c.y, 2);
> +
> +  var INT32_MAX = Math.pow(2, 31) - 1;

You can remove these two constants, they're defined in shell.js

@@ +20,5 @@
> +
> +  var INT32_MAX = Math.pow(2, 31) - 1;
> +  var INT32_MIN = -Math.pow(2, 31);
> +
> +  var d = int32x4(INT32_MAX, INT32_MIN, 0, 0);

nice

::: js/src/tests/ecma_6/TypedObject/simd/float64x2fromint32x4bits.js
@@ +16,5 @@
> +  var a = int32x4(0x00000000, 0x3ff00000, 0x0000000, 0x40000000);
> +  var c = float64x2.fromInt32x4Bits(a);
> +  assertEq(c.x, 1.0);
> +  assertEq(c.y, 2.0);
> +

can you add a second test where int32x4.{x,z} are not all 0?

::: js/src/tests/ecma_6/TypedObject/simd/float64x2sub.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 1031203;
> +var float64x2 = SIMD.float64x2;
> +
> +var summary = 'float64x2 sub';

You can delete this file, sub is already tested in float64x2-arithmetic.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat64x2.js
@@ +11,5 @@
> + */
> +
> +function test() {
> +  print(BUGNUMBER + ": " + summary);
> +

Here, we'd need as well a test for an inexact coercion from float64x2 to int32x4. Please write such a test and if it doesn't pass, comment it with a TODO please.

::: js/src/tests/ecma_6/TypedObject/simd/int32x4fromfloat64x2bits.js
@@ +32,5 @@
> +  assertEq(i.x, 0x00000000);
> +  assertEq(i.y, -0x80000000);
> +  assertEq(i.z, 0x00000000);
> +  assertEq(i.w, 0x7ff80000);
> +

Could you add a test case where the int32x4 .x and .z lanes aren't 0x0?

::: js/src/tests/ecma_6/TypedObject/simd/load.js
@@ -9,4 @@
>  const SIZE_ARRAY = 16;
>  
> -// 1 float32 == 4 bytes
> -const SIZE_BYTES = SIZE_ARRAY * 4;

You don't need to do this. If you keep a Float32Array of 16 elements, you can use it as a Float64Array of 8 elements. Please change this file accordingly.

@@ +32,4 @@
>        default:
>          assertEq(true, false, "unknown SIMD kind");
>      }
> +    var SIZE_BYTES = SIZE_ARRAY * sizeOfLaneElem;

(see comment at top of file -- you don't need this)

@@ +81,5 @@
> +
> +            load: function(index) {
> +               var v = SIMD[kind].load(arr, index);
> +               assertEqLanes(v, slice(index, 2), 16 / sizeOfLaneElem);
> +            }

Or: you can just define the loadXY and loadXYZ functions for all types, and assertEq(lanes > 2) in these two functions? I am not a big fan of this big if/else.

@@ +88,5 @@
>  }
>  
> +function testLoad(kind, TA, lanes) {
> +    var sizeOfLaneElem = 16 / lanes;
> +    var SIZE_BYTES = SIZE_ARRAY * sizeOfLaneElem;

(see comment at top of file -- you don't need this)

@@ +137,4 @@
>          C.loadX(lastValidArgLoadX);
>          assertThrowsInstanceOf(() => SIMD[kind].loadX(ta, lastValidArgLoadX + 1), TypeError);
>  
> +        if (lanes != 2) {

if (lanes > 2) {

@@ +162,2 @@
>  
> +    if (lanes == 4) {

You can merge this test with the previous one above.

::: js/src/tests/ecma_6/TypedObject/simd/shell.js
@@ +19,5 @@
>          throw e;
>      }
>  }
>  
> +function assertEqLanes(v, arr, lanes) {

Ideally, we don't want to have to give the number of lanes to this function call (neither do we in simdToArray). How about adding a small helper function in this file as well:

function simdLength(x) {
  if (typeof x.z !== 'undefined') return 4;
  return 2;
}

That works as SIMD objects aren't extensible. In the future, if we had e.g. Int16x8 (hint, we will), we'll probably have another named property accessor (let's say for the sake of example, 's') and we can just branch on that:

function simdLength(x) {
  if (typeof x.s !== 'undefined') return 8;
  if (typeof x.z !== 'undefined') return 4;
  return 2;
}

Then assertEqLanes (that I'd prefer to be named assertEqVec if you don't mind) can use simdLength, as can simdToArray below. And testBinaryFunc et al. can just avoid to give the 'lanes' argument. How does it sound?

@@ +28,5 @@
> +    else
> +        throw new TypeError("Unknown SIMD kind.");
> +}
> +
> +function simdToArray(v, lanes) {

See comment above (we can get rid of the 'lanes' parameter)

@@ +50,4 @@
>      var expected = varr.map(function(v, i) { return func(varr[i], warr[i]); });
>  
>      for (var i = 0; i < observed.length; i++)
> +        try { assertEq(observed[i], expected[i]); } catch(e) { print(e.stack); throw e; }

can you remove the try{} catch block please?

@@ +52,5 @@
>      for (var i = 0; i < observed.length; i++)
> +        try { assertEq(observed[i], expected[i]); } catch(e) { print(e.stack); throw e; }
> +}
> +
> +function testBinaryFuncCompare(v, w, simdFunc, func, inLanes) {

How about naming it testBinaryCompare?

@@ +59,5 @@
> +
> +    var observed = simdToArray(simdFunc(v, w), inLanes);
> +    var expected = [];
> +    for (var i = 0; i < 4; i++) {
> +        var j = (i * inLanes) / 4;

This is JS, not C++ :) For i = 1, this won't be an integer. I'd advise to use | 0 after the division to explicit the truncate (truncate is made by the index operation otherwise).

::: js/src/tests/ecma_6/TypedObject/simd/store.js
@@ +30,5 @@
>      reset(ta);
>      SIMD[kind].storeX(ta, i, v);
>      assertChanged(ta, i, [v.x]);
>  
> +    if (kind !== 'float64x2') {

(see big comment in shell.js first and then read this) You could test simdLength(v) > 2 here.

@@ +42,3 @@
>  
> +        reset(ta);
> +        SIMD[kind].store(ta, i, v);

This last test should be made for all kinds, including float64x2.

@@ +84,5 @@
> +    var F64 = new Float64Array(16);
> +
> +    var v = SIMD.float64x2(1, 2);
> +    testStore(F64, 'float64x2', 0, v);
> +    testStore(F64, 'float64x2', 1, v);

For this test and the ones below, can you please add

testStore(F64, 'float64x2', 14, v);

(which is, unless I'm wrong, the last possible index before an TypeError)

@@ +86,5 @@
> +    var v = SIMD.float64x2(1, 2);
> +    testStore(F64, 'float64x2', 0, v);
> +    testStore(F64, 'float64x2', 1, v);
> +
> +    var v = SIMD.float64x2(NaN, -0);

nice

::: js/src/tests/ecma_6/TypedObject/simd/swizzle-shuffle.js
@@ +23,5 @@
> +      case int32x4:
> +        return 4;
> +      case float64x2:
> +        return 2;
> +    }

can you throw, outside the switch, to make sure we don't silently escape this function and return undefined when we'll add new types?

@@ +33,4 @@
>  
>      assertThrowsInstanceOf(() => type.swizzle()               , TypeError);
>      assertThrowsInstanceOf(() => type.swizzle(v, 0)           , TypeError);
> +    if (lanes != 2) assertThrowsInstanceOf(() => type.swizzle(v, 0, 1), TypeError);

could you move this test as the "else" branch of the test below (if lanes == 2) { ... })

@@ +47,2 @@
>      // Test all possible swizzles.
> +    if (lanes == 4) {

I claim one could make these tests generic just by using the 'lanes' value here, and a smart combination of log2, pow, & and >> :)

@@ +51,5 @@
> +            [x, y, z, w] = [i & 3, (i >> 2) & 3, (i >> 4) & 3, (i >> 6) & 3];
> +            assertEqLanes(type.swizzle(v, x, y, z, w), swizzle4(simdToArray(v, lanes), x, y, z, w), lanes);
> +        }
> +    }
> +    else if (lanes == 2) {

style nit (here and below)

} else if (lanes == 2) {

@@ +55,5 @@
> +    else if (lanes == 2) {
> +        var x, y;
> +        for (var i = 0; i < Math.pow(2, 2); i++) {
> +          x = i % 2;
> +          y = i > 1 ? 1 : 0;

[x, y] = [i & 2, (i >> 1) & 2]

@@ +124,5 @@
>  
>  function testShuffleForType(type) {
> +    var lanes = getNumberOfLanesFromType(type);
> +    var lhs = lanes == 4 ? type(1,2,3,4) : type(1,2);
> +    var rhs = lanes == 4 ? type(5,6,7,8) : type(3,4);

can you just make it a single test?

var lhs, rhs;
if (lanes == 4) {
  lhs = type(1,2,3,4);
  rhs = type(5,6,7,8);
} else {
  assertEq(lanes, 2);
  lhs = type(1, 2);
  rhs = type(3, 4);
}

@@ +129,5 @@
>  
>      assertThrowsInstanceOf(() => type.shuffle(lhs)                   , TypeError);
>      assertThrowsInstanceOf(() => type.shuffle(lhs, rhs)              , TypeError);
>      assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0)           , TypeError);
> +    if (lanes != 2) assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 1) , TypeError);

can you move this test to the else branch of the if (lanes == 2) test below

@@ +134,5 @@
>      assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 1, 2)     , TypeError);
>      assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 1, 2, -1) , TypeError);
>      assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 1, 2, 8)  , TypeError);
>      assertThrowsInstanceOf(() => type.shuffle(lhs, 0, 1, 2, 7, rhs)  , TypeError);
> +    

nit: trailing whitespace

@@ +150,5 @@
> +            assertEqLanes(type.shuffle(lhs, rhs, x, y, z, w),
> +                          shuffle4(simdToArray(lhs, lanes), simdToArray(rhs, lanes), x, y, z, w), lanes);
> +        }
> +    }
> +    else if (lanes == 2) {

style nit: } else if ... {

@@ +153,5 @@
> +    }
> +    else if (lanes == 2) {
> +        var x, y;
> +        for (var i = 0; i < Math.pow(4, 2); i++) {
> +            [x, y] = [i & 3, (i >> 3) & 3];

nice

Comment 71

[Benjamin Bouvier [:bbouvier] (inactive)]

Actually, in the suggested simdLength function, instead of testing:

typeof vector.z !== 'undefined'

You can also test instead:

Object.getPrototypeOf(vector) == SIMD.int32x4.prototype // for int32x4

which appears to be *way* faster in quick benchmarking.

Comment 72

[:ProgramFOX]

Attachment: Implemented Float64x2

New patch, with the remarks fixed. Updated test cases will come soon.

> Actually, can't this value be 8?
Yes, that worked fine. I changed the constant.

Comment 73

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8534493 [details] [diff] [review]
Implemented Float64x2

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

\o/

Thanks a lot for this patch! I'd like to see the two main remarks fixed, but no need to re-ask for review -- that's r+. You can update your patch, add r=bbouvier at the end, send it to try, etc.

::: js/src/builtin/SIMD.cpp
@@ +366,2 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> +                             args.callee().getClass()->name, SimdTypeToMinimumLanesNumber(*descr), "s");

Strictly talking, the "s" should be added only if SimdTypeToMinimumLanesNumber > 1, but that's fine for now ;)

@@ +974,2 @@
>  static bool
> +Select(JSContext *cx, unsigned argc, Value *vp)

\o/

::: js/src/builtin/SIMD.h
@@ +109,5 @@
> +  V(sub, (BinaryFunc<Float64x2, Sub, Float64x2>), 2, 0)                             \
> +  V(withX, (FuncWith<Float64x2, WithX>), 2, 0)                                      \
> +  V(withY, (FuncWith<Float64x2, WithY>), 2, 0)                                      \
> +  V(withZ, (FuncWith<Float64x2, WithZ>), 2, 0)                                      \
> +  V(withW, (FuncWith<Float64x2, WithW>), 2, 0)

We don't have withZ and withW with Float64x2, please remove these ;)

::: js/src/builtin/TypedObjectConstants.h
@@ +44,5 @@
>  #define JS_DESCR_SLOT_STRUCT_FIELD_TYPES 8
>  #define JS_DESCR_SLOT_STRUCT_FIELD_OFFSETS 9
>  
> +// Slots for SIMD objects
> +#define JS_DESCR_SLOT_LANES              8

Can you please move this section just right below // Slots on scalars, references, and SIMD objects

Comment 74

[:ProgramFOX]

Attachment: Implemented Float64x2

New patch, with the main remarks fixed. Will push to try after finishing the test cases.

Comment 75

[:ProgramFOX]

Attachment: Added float64x2 test cases

Here is an updated patch for the test cases. I merged the files that could be merged -- aside from float64x2reciprocalsqrt.js, because it had a weird behaviour when merging it into float64x2-arithmetic.js (might due to use of float64s which cannot be exactly represented as a float32, but a Math.fround also didn't solve that).

> [x, y] = [i & 2, (i >> 1) & 2]
That didn't seem to work. I built the more generic formula on top of that, and it gave an error "invalid arguments", and when I tried it with the above non-generic formula, I got the same error. (I found some other formulas which made the tests pass, but I wasn't able to verify their correctness so I just kept it like it is for now).

> Or: you can just define the loadXY and loadXYZ functions for all types, and assertEq(lanes > 2) in these two functions? I am not a big fan of this big if/else.
Instead of using the assert, I put a `if (lanes < 4) return;` statement in the C.LoadXY and C.LoadXYZ functions. By using that, I could make the if statement below smaller. Now, C.LoadXY(...) can also be executed even if the SIMD type doesn't have a LoadXY function, because it returns if it does not. Please let me know what you think about this.

Comment 76

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8536202 [details] [diff] [review]
Added float64x2 test cases

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

Could you make sure that the SIMD tests don't get ultra-slow? Even though I am the one who proposed it, I am actually unsure about the assertVec way to do things. In particular, can you try |jstests.py ./js SIMD| before and after this patch?

Also, there is something wrong with select, either in this patch or the previous one.

::: js/src/tests/ecma_7/SIMD/float32x4fromfloat64x2.js
@@ +41,5 @@
> +  assertEq(m.x, Math.fround(j));
> +  assertEq(m.y, Math.fround(k));
> +  assertEq(m.z, 0);
> +  assertEq(m.w, 0);
> +

Can you add a test with SIMD.float64x2(Math.pow(2, 1000), Math.pow(2, -1000))?

::: js/src/tests/ecma_7/SIMD/float64x2-arithmetic.js
@@ +30,5 @@
> +}
> +function testAbs(v) {
> +    return testUnaryFunc(v, float64x2.abs, Math.abs);
> +}
> +function testReciprocal(v) {

Did you forget about testReciprocalSqrt? The file contains the JS helper, but not the test* counterpart.

@@ +51,5 @@
> +      testSub(v, w);
> +      testMul(v, w);
> +      testDiv(v, w);
> +      testAbs(v);
> +      testAbs(w);

No need to call testAbs, testReciprocal, testSqrt twice ;)

::: js/src/tests/ecma_7/SIMD/float64x2neg.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 1031203;
> +var float64x2 = SIMD.float64x2;
> +
> +var summary = 'float64x2 neg';

Can you add it to float64x2-arithmetic as well, and delete this file?

::: js/src/tests/ecma_7/SIMD/float64x2reciprocalsqrt.js
@@ +1,5 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 1031203;
> +var float64x2 = SIMD.float64x2;
> +
> +var summary = 'float64x2 reciprocalSqrt';

Can you delete this test file, and test reciprocalSqrt in float64x2-arithmetic instead, please?

::: js/src/tests/ecma_7/SIMD/float64x2select.js
@@ +17,5 @@
> +  var b = SIMD.float64x2(1, 2);
> +  var c = SIMD.float64x2(3, 4);
> +  var d = SIMD.float64x2.select(a, b, c);
> +  assertEq(d.x, 1);
> +  assertEq(d.y, 4);

Well, either this test doesn't pass, or the implementation is incorrect here (if that's the case, sorry for not having spotted this in your previous patch): as a.y is true, d.y should be equal to b.y, i.e.

assertEq(d.y, 2);

@@ +23,5 @@
> +  var e = SIMD.float64x2(NaN, -0);
> +  var f = SIMD.float64x2(+Infinity, -Infinity);
> +  var g = SIMD.float64x2.select(a, e, f);
> +  assertEq(g.x, NaN);
> +  assertEq(g.y, -Infinity);

Same here, g.y === -0

::: js/src/tests/ecma_7/SIMD/int32x4fromfloat64x2.js
@@ +22,5 @@
> +
> +  var d = float64x2(NaN, -0);
> +  var f = int32x4.fromFloat64x2(d);
> +  assertEq(f.x, 0);
> +  assertEq(f.y, 0);

can you check that the two last lanes are 0 in all test, please?

::: js/src/tests/ecma_7/SIMD/load.js
@@ +151,2 @@
>  
> +    if (lanes == 4) {

Please merge the two |if| bodies into one.

::: js/src/tests/ecma_7/SIMD/shell.js
@@ +29,5 @@
> +    else
> +        throw new TypeError("Unknown SIMD kind.");
> +}
> +
> +function simdLength(v) {

Could you move it above assertEqVec, please?

@@ +61,5 @@
> +    var observed = simdToArray(simdFunc(v));
> +    var expected = varr.map(function(v, i) { return func(varr[i]); });
> +
> +    for (var i = 0; i < observed.length; i++)
> +        try { assertEq(observed[i], expected[i]); } catch(e) { print(e.stack); throw e; }

Can you remove the try/catch?

@@ +85,5 @@
> +    var expected = [];
> +    for (var i = 0; i < 4; i++) {
> +        var j = ((i * inLanes) / 4) | 0;
> +        expected.push(func(varr[j], warr[j]));
> +    }

Or you could also just create the |expected| array in this function and then call testBinaryFunc, but it's fine as is.

@@ +86,5 @@
> +    for (var i = 0; i < 4; i++) {
> +        var j = ((i * inLanes) / 4) | 0;
> +        expected.push(func(varr[j], warr[j]));
> +    }
> +    for (var x = 0; x < observed.length; x++)

Please replace observed.length by 4, as the result should be an int32x4. Can you also assertEq(observed.length, 4); above the loops?

::: js/src/tests/ecma_7/SIMD/swizzle-shuffle.js
@@ +41,5 @@
>  
> +    if (lanes == 2) {
> +        assertThrowsInstanceOf(() => type.swizzle(v, 0, -1)   , TypeError);
> +        assertThrowsInstanceOf(() => type.swizzle(v, 0, 2)    , TypeError);
> +    } else {

Can you add

assertEq(lanes, 4);

@@ +53,5 @@
> +            [x, y, z, w] = [i & 3, (i >> 2) & 3, (i >> 4) & 3, (i >> 6) & 3];
> +            assertEqVec(type.swizzle(v, x, y, z, w), swizzle4(simdToArray(v), x, y, z, w));
> +        }
> +    }
> +    else if (lanes == 2) {

Can you change this to 

} else {
  assertEq(lanes, 2);

@@ +71,5 @@
>      };
> +    if (lanes == 4) {
> +        assertEqVec(type.swizzle(v, obj, obj, obj, obj), swizzle4(simdToArray(v), 0, 1, 2, 3));
> +    }
> +    else if (lanes == 2) {

Can you change this to:

} else {
  assertEq(lanes, 2);

@@ +147,5 @@
> +    if (lanes == 2) {
> +        assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 4)    , TypeError);
> +        assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, -1)   , TypeError);
> +    } else {
> +        assertThrowsInstanceOf(() => type.shuffle(lhs, rhs, 0, 1) , TypeError);

Can you add above

assertEq(lanes, 4);

@@ +157,5 @@
> +        var x, y, z, w;
> +        for (var i = 0; i < Math.pow(8, 4); i++) {
> +            [x, y, z, w] = [i & 7, (i >> 3) & 7, (i >> 6) & 7, (i >> 9) & 7];
> +            assertEqVec(type.shuffle(lhs, rhs, x, y, z, w),
> +                          shuffle4(simdToArray(lhs), simdToArray(rhs), x, y, z, w));

please align the arguments vertically:

assertEqVec(type.shuffle(lhs, rhs, x, y, z, w),
            shuffle4(simdToArray(lhs), simdToArray(rhs), x, y, z, w));

@@ +159,5 @@
> +            [x, y, z, w] = [i & 7, (i >> 3) & 7, (i >> 6) & 7, (i >> 9) & 7];
> +            assertEqVec(type.shuffle(lhs, rhs, x, y, z, w),
> +                          shuffle4(simdToArray(lhs), simdToArray(rhs), x, y, z, w));
> +        }
> +    } else if (lanes == 2) {

} else {
  assertEq(lanes, 2);

@@ +164,5 @@
> +        var x, y;
> +        for (var i = 0; i < Math.pow(4, 2); i++) {
> +            [x, y] = [i & 3, (i >> 3) & 3];
> +            assertEqVec(type.shuffle(lhs, rhs, x, y),
> +                          shuffle2(simdToArray(lhs), simdToArray(rhs), x, y));

same here (vertical alignment)

@@ +176,5 @@
>          valueOf: function() { return this.x++ }
>      };
> +    if (lanes == 4) {
> +        assertEqVec(type.shuffle(lhs, rhs, obj, obj, obj, obj),
> +                      shuffle4(simdToArray(lhs),simdToArray(rhs), 0, 1, 2, 3));

same here (vertical alignment)

@@ +178,5 @@
> +    if (lanes == 4) {
> +        assertEqVec(type.shuffle(lhs, rhs, obj, obj, obj, obj),
> +                      shuffle4(simdToArray(lhs),simdToArray(rhs), 0, 1, 2, 3));
> +    }
> +    else if (lanes == 2) {

} else {
  assertEq(lanes, 2);

Comment 77

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8536202 [details] [diff] [review]
Added float64x2 test cases

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

Nevermind my comment about select: this is because what's now called "select" in our interpreter is actually named "bitselect" in the spec. Consider this a r+ if you address all comments from the previous review ;)

Comment 78

[:ProgramFOX]

Attachment: Implemented Float64x2

I had to rebase this, so here is the updated patch.

(I was not sure whether I had to r? again because it was already r+'ed, so I just did it to be sure)

Comment 79

[:ProgramFOX]

Attachment: Added float64x2 test cases

Updated test cases, with the comments fixed.

> Could you make sure that the SIMD tests don't get ultra-slow?

Can this please be done in a follow-up bug? I have tried a few things, but none of them speeded up the test cases.

Comment 80

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8539446 [details] [diff] [review]
Added float64x2 test cases

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

(In reply to ProgramFOX from comment #79)
> Created attachment 8539446 [details] [diff] [review]
> Added float64x2 test cases
> 
> Updated test cases, with the comments fixed.
> 
> > Could you make sure that the SIMD tests don't get ultra-slow?
> 
> Can this please be done in a follow-up bug? I have tried a few things, but
> none of them speeded up the test cases.

No. To be more precise, I am not asking for making tests faster, I am just asking what's the difference between running all SIMD tests before this patch, and after this patch. Do you think you can provide these numbers?

Comment 81

[:ProgramFOX]

Sure.

Tests before patches applied: 13.8s (for 50 tests)
Tests after patches applied: 17.9s (for 66 tests)

I've run the tests on the shell with a debug build.

Comment 82

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8539445 [details] [diff] [review]
Implemented Float64x2

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

When rebasing is trivial and the patch is already r+'d, no need to ask for another round of review ;)

Comment 83

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8539446 [details] [diff] [review]
Added float64x2 test cases

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

Thanks again (already r+ in the previous review round, if you look carefully ;)). The slowdown seems acceptable, and I have ideas on how to speed things up here, so we're fine.

::: js/src/tests/ecma_7/SIMD/shell.js
@@ +38,5 @@
> +    else if (lanes == 2)
> +        assertEqX2(v, arr);
> +    else
> +        throw new TypeError("Unknown SIMD kind.");
> +}          

nit: trailing whitespaces at the end of this line

@@ +88,5 @@
> +        var j = ((i * inLanes) / 4) | 0;
> +        expected.push(func(varr[j], warr[j]));
> +    }
> +    for (var x = 0; x < 4; x++)
> +        assertEq(observed[i], expected[i]);

You can actually replace these two loops with

for (var i = 0; i < 4; i++) {
  var j = ((i * inLanes) / 4) | 0;
  assertEq(func(varr[j], warr[j]), expected[i]);
}

Comment 84

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8539446 [details] [diff] [review]
Added float64x2 test cases

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

load.js seems to timeout when locally run with --tbpl, so you'll actually need to speed things up if you want to get this in tree. A few ideas below. If that's not enough, ping me on IRC and we'll figure out something.

::: js/src/tests/ecma_7/SIMD/load.js
@@ +51,4 @@
>          assertEq(asArray.length, SIZE_BYTES);
>  
>          return new typedArrayCtor(new Uint8Array(asArray).buffer);
>      }

var assertFunc = (lanes == 2) ? assertEqX2 : assertEqX4;
var type = SIMD[kind];

@@ +54,5 @@
>      }
>  
>      return {
>          loadX: function(index) {
>              var v = SIMD[kind].loadX(arr, index);

s/SIMD[kind]/type here and below

@@ +55,5 @@
>  
>      return {
>          loadX: function(index) {
>              var v = SIMD[kind].loadX(arr, index);
> +            assertEqVec(v, slice(index, 1));

assertFunc(...) here and below

Comment 85

[:ProgramFOX]

Attachment: Implemented Float64x2

New patch, just to update the commit message.

Comment 86

[:ProgramFOX]

Attachment: Added float64x2 test cases

Updated patch. The speed-ups for load.js worked fine. I also added r=bbouvier to the commit message.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0b8053e82d

Comment 87

[Benjamin Bouvier [:bbouvier] (inactive)]

Try build looks good! Congrats!

Comment 88

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/232c0f30075c
https://hg.mozilla.org/integration/mozilla-inbound/rev/776c989112f1

Comment 89

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4906949&repo=mozilla-inbound

Comment 90

[Benjamin Bouvier [:bbouvier] (inactive)]

Ha fun, an alignment issue in C++ code. Probably some bad C++ optimization made by the compiler there, as it doesn't show up anywhere else. ProgramFOX, let's see what we can do here.

When the trees open up, I'll post a Try-build with printf statements to identify where the error shows up precisely.

Comment 91

[Benjamin Bouvier [:bbouvier] (inactive)]

Try is open:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4fe7ffadec78

Comment 92

[:ProgramFOX]

(In reply to Benjamin Bouvier [:bbouvier] from comment #91)
> Try is open:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4fe7ffadec78

All builds are busted:

> /builds/slave/try-and-api-9-0000000000000000/build/js/src/builtin/SIMD.cpp:1006:1: error: pasting "," and "__VA_ARGS__" does not give a valid preprocessing token

Comment 93

[Benjamin Bouvier [:bbouvier] (inactive)]

Weird, that debug code has been taken from other parts of the tree...

- with args instead of __VA_ARGS__
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1db5e860b3a8
- with __VA_ARGS__ but not ## (suggested by padenot)
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e96b5221ee6f

Comment 94

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 95

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/932463bf1cde
https://hg.mozilla.org/projects/cypress/rev/870e10e9bc55

Comment 96

[Benjamin Bouvier [:bbouvier] (inactive)]

Actually the issue is silly: on ARM, explicitly loading a double needs the data to be aligned on 8 bytes boundaries, so we can't rely on simple element copy. Let's try something more dynamic like memcpy, which can optimize the nice cases (and Just Work (c) in all other cases):

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

Comment 97

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 98

[Benjamin Bouvier [:bbouvier] (inactive)]

ProgramFOX, the try build (at least on android platforms) looks good. Can you replace the |for| loops in Load/Save with calls to memcpy (as done in [0]), submit the updated patch (no need for r?) and run another try build (without all the debug printf), please?

[0] https://hg.mozilla.org/try/rev/7b25449a1fdf

Comment 99

[:ProgramFOX]

Attachment: Implemented Float64x2

Updated patch, with memcpy. Still r?'ing because I had to do heavy rebasing.

Comment 100

[:ProgramFOX]

Attachment: Added float64x2 test cases

Updated test cases. In select-bitselect.js it didn't work initially as I said on IRC. There was an error in the JavaScript function bitselect, and if ifFalse contained -NaN it failed. How did I solve this? I took parts from the polyfill's bitselect method and ported those to the test case, instead of using the bit operations.

Comment 101

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8542600 [details] [diff] [review]
Implemented Float64x2

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

Still looks good :)

::: js/src/builtin/SIMD.cpp
@@ +382,5 @@
>          break;
>        }
> +      case SimdTypeDescr::TYPE_FLOAT64: {
> +        double *mem = reinterpret_cast<double*>(result->typedMem());
> +        for (unsigned i = 0; i < 2; i++)

nit: {} for the for loop body

Comment 102

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8542601 [details] [diff] [review]
Added float64x2 test cases

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

Alright, thanks.  Sorry for the late review.  Let's try this and land it!

::: js/src/tests/ecma_7/SIMD/clamp.js
@@ +1,2 @@
> +// |reftest| skip-if(!this.hasOwnProperty("SIMD"))
> +var BUGNUMBER = 946042;

Please drop references to BUGNUMBER in all files, or fix them accordingly.

::: js/src/tests/ecma_7/SIMD/swizzle-shuffle.js
@@ +58,5 @@
> +        assertEq(lanes, 2);
> +        var x, y;
> +        for (var i = 0; i < Math.pow(2, 2); i++) {
> +          x = i % 2;
> +          y = i > 1 ? 1 : 0;

please replace these two lines by the equivalent form [x, y] = [x & 1, (y >> 1) & 1];

@@ +179,5 @@
>          valueOf: function() { return this.x++ }
>      };
> +    if (lanes == 4) {
> +        assertEqVec(type.shuffle(lhs, rhs, obj, obj, obj, obj),
> +                    shuffle4(simdToArray(lhs),simdToArray(rhs), 0, 1, 2, 3));

can you add spaces between simdToArray(lhs),simdToArray(rhs) like this:

simdToArray(lhs), simdToArray(rhs)

@@ +183,5 @@
> +                    shuffle4(simdToArray(lhs),simdToArray(rhs), 0, 1, 2, 3));
> +    } else {
> +        assertEq(lanes, 2);
> +        assertEqVec(type.shuffle(lhs, rhs, obj, obj),
> +                    shuffle2(simdToArray(lhs),simdToArray(rhs), 0, 1));

ditto

Comment 103

[:ProgramFOX]

Attachment: Implemented Float64x2

Updated patch, with a one-line rebasing and the remark fixed.

Comment 104

[:ProgramFOX]

Attachment: Added float64x2 test cases

Updated test case patch, with the remarks fixed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2b4cd165e2

Comment 105

[Benjamin Bouvier [:bbouvier] (inactive)]

Looks quite green to me, let's try to land it

Comment 106

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7ee9cb2247
https://hg.mozilla.org/integration/mozilla-inbound/rev/178c7164a3cd

Comment 107

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8549058 [details] [diff] [review]
Implemented Float64x2

For future reference, it's very-much appreciated to set the checkin+ flag on patches that have previously landed in a bug when everything isn't landing at the same time. Makes things way less confusing when handling checkin-needed :)

Comment 108

[Ryan VanderMeulen [:RyanVM]]

#jsapi tells me that this apparently caused bustage due to conflicts with bug 1112778. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f7a19c5572

../../../mozilla/js/src/builtin/SIMD.cpp:451:20: error: no member named 'defineProperty' in 'JSObject'

Comment 109

[:ProgramFOX]

Attachment: Implemented Float64x2

Fixed. Because I only had to change one line to correct the name of a used function, I'll already r+.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ea82c66a286

Comment 110

[:ProgramFOX]

Attachment: Added float64x2 test cases

I did not have to rebase the test cases, but I'll still upload an updated patch, to make sure the commit date of the test cases is later than the commit date of the implementation patch. It would be very confusing if the date for the test cases would be earlier, as the implementation patch has to be applied first.

Comment 111

[:ProgramFOX]

Try push is green, adding checkin-needed.

Comment 112

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce5ad5a7539
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1118794ba7c

Comment 113

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ce5ad5a7539
https://hg.mozilla.org/mozilla-central/rev/a1118794ba7c

Comment 114

[Florian Scholz (Open Web Docs)]

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