Expose SIMD.Float32x4 type to Javascript.

Expose SIMD.Float32x4 type to Javascript.
This CL exposes the constructor function, defines type related
information, and implements value type semantics.
It also refactors test/mjsunit/samevalue.js to test SameValue and SameValueZero.

TEST=test/mjsunit/harmony/simd.js, test/cctest/test-simd.cc

LOG=Y
BUG=v8:4124
Committed: https://crrev.com/e5ed3bee99807c502fa7d7a367ec401e16d3f773
Cr-Commit-Position: refs/heads/master@{#29689}

Committed: https://crrev.com/61130584270ebffdfc43c4ab802c6b0b75f1504b
Cr-Commit-Position: refs/heads/master@{#29712}

[bbudge, 2015-07-01 00:06:52]

bbudge@chromium.org changed reviewers:
+ rossberg@chromium.org

[bbudge, 2015-07-01 00:06:53]

There are a couple of issues still but I think it's ready for a look.

1) Running with --always-opt makes it impossible to access functions on the
SIMD.float32x4 object.
2) Some of the other tests need the --harmony-simd flag. I could fix those but
I'm not sure whether those extra tests should be included in this CL.
3) There are a lot of tests that could potentially have SIMD stuff added to
them, like the mirror debugger stuff. Which ones should I include here?
4) Is this CL too big? It's not clear how the different parts interact, so I'm
not sure how to decompose this into smaller CLs. Many of the changes are pretty
formulaic (code generators.)

[bbudge, 2015-07-01 05:41:18]

On 2015/07/01 00:06:53, bbudge wrote:
> There are a couple of issues still but I think it's ready for a look.
> 
> 1) Running with --always-opt makes it impossible to access functions on the
> SIMD.float32x4 object.
> 2) Some of the other tests need the --harmony-simd flag. I could fix those but
> I'm not sure whether those extra tests should be included in this CL.
> 3) There are a lot of tests that could potentially have SIMD stuff added to
> them, like the mirror debugger stuff. Which ones should I include here?
> 4) Is this CL too big? It's not clear how the different parts interact, so I'm
> not sure how to decompose this into smaller CLs. Many of the changes are
pretty
> formulaic (code generators.)

Re #1 it seems to require --turbo and --always-opt to fail.

[rossberg, 2015-07-02 13:35:44]

On 2015/07/01 00:06:53, bbudge wrote:
> There are a couple of issues still but I think it's ready for a look.
> 
> 1) Running with --always-opt makes it impossible to access functions on the
> SIMD.float32x4 object.

If this is with --turbo only, then you probably still need to handle SIMD values
in TF somewhere. Obviously, this CL doesn't contain any TF code. (It's fine if
that's a follow-up, though.)

> 2) Some of the other tests need the --harmony-simd flag. I could fix those but
> I'm not sure whether those extra tests should be included in this CL.

Which other tests do you mean? Are you saying that there is code not using SIMD
that now breaks? If so, that's bad.

> 3) There are a lot of tests that could potentially have SIMD stuff added to
> them, like the mirror debugger stuff. Which ones should I include here?

The debugger should definitely have some test coverage. As for others tests, I'm
not sure. Any particular ones you wonder about?

> 4) Is this CL too big? It's not clear how the different parts interact, so I'm
> not sure how to decompose this into smaller CLs. Many of the changes are
pretty
> formulaic (code generators.)

Yeah, it's pretty big, but I can cope. :)

https://codereview.chromium.org/1219943002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1219943002/diff/1/include/v8.h#newcode3973
include/v8.h:3973: * A Float32x4 object (ECMA-262 edition 7).
Nit: point to draft instead

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc#newcode951
src/code-stubs.cc:951: if (s.Contains(ToBooleanStub::SIMD_TYPE))
p.Add("SimdType");
Nit: "Simd"

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h#newcode2753
src/code-stubs.h:2753: SIMD_TYPE,
Nit: simply SIMD would seem more consistent with the other names.

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h#newcode84
src/contexts.h:84: V(SIMD_OBJECT_INDEX, JSObject, simd_object)                  
               \
Is this actually used?

https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js
File src/harmony-simd.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js#newcode29
src/harmony-simd.js:29: function Float32x4SplatConstructor(s) {
Nit: just 'Float32x4Splat' is more consistent

https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator...
src/heap-snapshot-generator.cc:153: return "/simd/";
Nit: consistent layout

https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js#newc...
src/mirror-debugger.js:634: %_CallFunction(this, Float32x4_TYPE, value,
ValueMirror);
Isn't this spelled FLOAT32X4_TYPE?

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode126
src/runtime.js:126: } else if (IS_SYMBOL(x) || IS_FLOAT32X4(x)) {
This seems wrong. There needs to be a separate case for FLOAT32X4, which
ultimately has to perform a structural comparison.

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode154
src/runtime.js:154: STRICT_EQUALS = function STRICT_EQUALS(x) {
This needs a new case for simds as well.

https://codereview.chromium.org/1219943002/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode231
src/types.cc:231: // TODO(bbudge): Add type bits for SIMD value types.
Nit: move down comment by one line

https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode299
src/types.cc:299: if (value->IsFloat32x4()) return kAny;
This shouldn't be needed, since it's covered by the call to Lub(Map*).

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:242: Handle<Object> handle = factory->NewFloat32x4(1,
2, 3, 4);
Type this as Handle<Float32x4> and get rid of the separate declaration for
'value' and the cast it involves.

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:245: 
Also check that the lanes have the values passed to the constructor.

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:254: CHECK_EQ(-0.0, value->get_lane(1));
Does CHECK_EQ actually distinguish -0.0 from +0.0?

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.js
File test/mjsunit/harmony/simd.js (right):

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:46: 
Add tests for the SIMD object itself, e.g.:

assertSame(typeof SIMD, 'object');
assertSame(SIMD.constructor, Object);
assertSame(Object.getPrototypeOf(SIMD), Object.prototype);
assertSame(SIMD + "", "[object SIMD]");

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:68: assertSame(simdFn,
Object(values[i]).__proto__.constructor)
assertSame(simdFn.prototype, value[i].__proto__)  etc

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:158: // Every SIMD value should equal itself, and
non-strictly equal its wrapper.
This is lacking tests that make sure that equality is properly structural:

SIMD.float32x4(1, 2, 3, 4) == SIMD.float32x4(1, 2, 3, 4)
SIMD.float32x4(1, 2, 3, 4) === SIMD.float32x4(1, 2, 3, 4)

which probably doesn't work correctly yet.

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js
File test/mjsunit/messages.js (right):

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js#ne...
test/mjsunit/messages.js:328: 1 + SIMD.Float32x4(1, 2, 3, 4);
Typo: SIMD.float32x4 (you got a TypeError for the wrong reason)

[bbudge, 2015-07-06 23:59:06]

I backed out changes to mirror-debugger.js for another CL. I think I addressed
all of your comments. PTAL

https://codereview.chromium.org/1219943002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1219943002/diff/1/include/v8.h#newcode3973
include/v8.h:3973: * A Float32x4 object (ECMA-262 edition 7).
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: point to draft instead

Done.

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc#newcode951
src/code-stubs.cc:951: if (s.Contains(ToBooleanStub::SIMD_TYPE))
p.Add("SimdType");
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: "Simd"

SimdValue?

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h#newcode2753
src/code-stubs.h:2753: SIMD_TYPE,
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: simply SIMD would seem more consistent with the other names.

How about SIMD_VALUE, since SIMD is more of an adjective than a noun?

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h#newcode84
src/contexts.h:84: V(SIMD_OBJECT_INDEX, JSObject, simd_object)                  
               \
On 2015/07/02 13:35:43, rossberg wrote:
> Is this actually used?

Turns out it's needed for bootstrapper.cc:1329:
native_context()->set_simd_object(*simd_object);

https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js
File src/harmony-simd.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js#newcode29
src/harmony-simd.js:29: function Float32x4SplatConstructor(s) {
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: just 'Float32x4Splat' is more consistent

Done.

https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator.cc
File src/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator...
src/heap-snapshot-generator.cc:153: return "/simd/";
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: consistent layout

Done (but this is git cl format)

https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js#newc...
src/mirror-debugger.js:634: %_CallFunction(this, Float32x4_TYPE, value,
ValueMirror);
On 2015/07/02 13:35:43, rossberg wrote:
> Isn't this spelled FLOAT32X4_TYPE?

Yes, thanks.

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode126
src/runtime.js:126: } else if (IS_SYMBOL(x) || IS_FLOAT32X4(x)) {
On 2015/07/02 13:35:43, rossberg wrote:
> This seems wrong. There needs to be a separate case for FLOAT32X4, which
> ultimately has to perform a structural comparison.

I misunderstood the spec to state that comparisons should return false. Fixed to
do a componentwise equality test.

https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode154
src/runtime.js:154: STRICT_EQUALS = function STRICT_EQUALS(x) {
On 2015/07/02 13:35:43, rossberg wrote:
> This needs a new case for simds as well.

Done.

https://codereview.chromium.org/1219943002/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode231
src/types.cc:231: // TODO(bbudge): Add type bits for SIMD value types.
On 2015/07/02 13:35:43, rossberg wrote:
> Nit: move down comment by one line

Done.

https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode299
src/types.cc:299: if (value->IsFloat32x4()) return kAny;
On 2015/07/02 13:35:43, rossberg wrote:
> This shouldn't be needed, since it's covered by the call to Lub(Map*).

Done.

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:242: Handle<Object> handle = factory->NewFloat32x4(1,
2, 3, 4);
On 2015/07/02 13:35:43, rossberg wrote:
> Type this as Handle<Float32x4> and get rid of the separate declaration for
> 'value' and the cast it involves.

Done.

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:245: 
On 2015/07/02 13:35:43, rossberg wrote:
> Also check that the lanes have the values passed to the constructor.

Done.

https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
test/cctest/test-heap.cc:254: CHECK_EQ(-0.0, value->get_lane(1));
On 2015/07/02 13:35:43, rossberg wrote:
> Does CHECK_EQ actually distinguish -0.0 from +0.0?

Good point - probably not. I added a test for the sign bit.

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.js
File test/mjsunit/harmony/simd.js (right):

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:46: 
On 2015/07/02 13:35:43, rossberg wrote:
> Add tests for the SIMD object itself, e.g.:
> 
> assertSame(typeof SIMD, 'object');
> assertSame(SIMD.constructor, Object);
> assertSame(Object.getPrototypeOf(SIMD), Object.prototype);
> assertSame(SIMD + "", "[object SIMD]");

Thanks. Done.

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:68: assertSame(simdFn,
Object(values[i]).__proto__.constructor)
On 2015/07/02 13:35:43, rossberg wrote:
> assertSame(simdFn.prototype, value[i].__proto__)  etc

Done.

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
test/mjsunit/harmony/simd.js:158: // Every SIMD value should equal itself, and
non-strictly equal its wrapper.
On 2015/07/02 13:35:43, rossberg wrote:
> This is lacking tests that make sure that equality is properly structural:
> 
> SIMD.float32x4(1, 2, 3, 4) == SIMD.float32x4(1, 2, 3, 4)
> SIMD.float32x4(1, 2, 3, 4) === SIMD.float32x4(1, 2, 3, 4)
> 
> which probably doesn't work correctly yet.

Added tests for structural equality semantics.

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js
File test/mjsunit/messages.js (right):

https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js#ne...
test/mjsunit/messages.js:328: 1 + SIMD.Float32x4(1, 2, 3, 4);
On 2015/07/02 13:35:44, rossberg wrote:
> Typo: SIMD.float32x4 (you got a TypeError for the wrong reason)

Done.

[bbudge, 2015-07-07 00:04:02]

On 2015/07/02 13:35:44, rossberg wrote:
> On 2015/07/01 00:06:53, bbudge wrote:
> > There are a couple of issues still but I think it's ready for a look.
> > 
> > 1) Running with --always-opt makes it impossible to access functions on the
> > SIMD.float32x4 object.
> 
> If this is with --turbo only, then you probably still need to handle SIMD
values
> in TF somewhere. Obviously, this CL doesn't contain any TF code. (It's fine if
> that's a follow-up, though.)

OK, I got my tests to pass for now by specifying the --noalways-opt flag for
simd.js.

> 
> > 2) Some of the other tests need the --harmony-simd flag. I could fix those
but
> > I'm not sure whether those extra tests should be included in this CL.
> 
> Which other tests do you mean? Are you saying that there is code not using
SIMD
> that now breaks? If so, that's bad.

My changes to object-toprimitive.js and es6/reflect-apply.js require the flag.
> 
> > 3) There are a lot of tests that could potentially have SIMD stuff added to
> > them, like the mirror debugger stuff. Which ones should I include here?
> 
> The debugger should definitely have some test coverage. As for others tests,
I'm
> not sure. Any particular ones you wonder about?

Yeah, reflect-apply and object-toprimitive. BTW I backed out my changes to
mirror-debugger.js for now. It seems like it should be a follow-on patch.
> 
> > 4) Is this CL too big? It's not clear how the different parts interact, so
I'm
> > not sure how to decompose this into smaller CLs. Many of the changes are
> pretty
> > formulaic (code generators.)
> 
> Yeah, it's pretty big, but I can cope. :)
> 
> https://codereview.chromium.org/1219943002/diff/1/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/include/v8.h#newcode3973
> include/v8.h:3973: * A Float32x4 object (ECMA-262 edition 7).
> Nit: point to draft instead
> 
> https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc
> File src/code-stubs.cc (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.cc#newcode951
> src/code-stubs.cc:951: if (s.Contains(ToBooleanStub::SIMD_TYPE))
> p.Add("SimdType");
> Nit: "Simd"
> 
> https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h
> File src/code-stubs.h (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h#newcode2753
> src/code-stubs.h:2753: SIMD_TYPE,
> Nit: simply SIMD would seem more consistent with the other names.
> 
> https://codereview.chromium.org/1219943002/diff/1/src/contexts.h
> File src/contexts.h (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/src/contexts.h#newcode84
> src/contexts.h:84: V(SIMD_OBJECT_INDEX, JSObject, simd_object)                
 
>                \
> Is this actually used?
> 
> https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js
> File src/harmony-simd.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/src/harmony-simd.js#newcode29
> src/harmony-simd.js:29: function Float32x4SplatConstructor(s) {
> Nit: just 'Float32x4Splat' is more consistent
> 
>
https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator.cc
> File src/heap-snapshot-generator.cc (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/src/heap-snapshot-generator...
> src/heap-snapshot-generator.cc:153: return "/simd/";
> Nit: consistent layout
> 
> https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js
> File src/mirror-debugger.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/src/mirror-debugger.js#newc...
> src/mirror-debugger.js:634: %_CallFunction(this, Float32x4_TYPE, value,
> ValueMirror);
> Isn't this spelled FLOAT32X4_TYPE?
> 
> https://codereview.chromium.org/1219943002/diff/1/src/runtime.js
> File src/runtime.js (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode126
> src/runtime.js:126: } else if (IS_SYMBOL(x) || IS_FLOAT32X4(x)) {
> This seems wrong. There needs to be a separate case for FLOAT32X4, which
> ultimately has to perform a structural comparison.
> 
> https://codereview.chromium.org/1219943002/diff/1/src/runtime.js#newcode154
> src/runtime.js:154: STRICT_EQUALS = function STRICT_EQUALS(x) {
> This needs a new case for simds as well.
> 
> https://codereview.chromium.org/1219943002/diff/1/src/types.cc
> File src/types.cc (right):
> 
> https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode231
> src/types.cc:231: // TODO(bbudge): Add type bits for SIMD value types.
> Nit: move down comment by one line
> 
> https://codereview.chromium.org/1219943002/diff/1/src/types.cc#newcode299
> src/types.cc:299: if (value->IsFloat32x4()) return kAny;
> This shouldn't be needed, since it's covered by the call to Lub(Map*).
> 
> https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc
> File test/cctest/test-heap.cc (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
> test/cctest/test-heap.cc:242: Handle<Object> handle = factory->NewFloat32x4(1,
> 2, 3, 4);
> Type this as Handle<Float32x4> and get rid of the separate declaration for
> 'value' and the cast it involves.
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
> test/cctest/test-heap.cc:245: 
> Also check that the lanes have the values passed to the constructor.
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/cctest/test-heap.cc#ne...
> test/cctest/test-heap.cc:254: CHECK_EQ(-0.0, value->get_lane(1));
> Does CHECK_EQ actually distinguish -0.0 from +0.0?
> 
> https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.js
> File test/mjsunit/harmony/simd.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
> test/mjsunit/harmony/simd.js:46: 
> Add tests for the SIMD object itself, e.g.:
> 
> assertSame(typeof SIMD, 'object');
> assertSame(SIMD.constructor, Object);
> assertSame(Object.getPrototypeOf(SIMD), Object.prototype);
> assertSame(SIMD + "", "[object SIMD]");
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
> test/mjsunit/harmony/simd.js:68: assertSame(simdFn,
> Object(values[i]).__proto__.constructor)
> assertSame(simdFn.prototype, value[i].__proto__)  etc
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/harmony/simd.j...
> test/mjsunit/harmony/simd.js:158: // Every SIMD value should equal itself, and
> non-strictly equal its wrapper.
> This is lacking tests that make sure that equality is properly structural:
> 
> SIMD.float32x4(1, 2, 3, 4) == SIMD.float32x4(1, 2, 3, 4)
> SIMD.float32x4(1, 2, 3, 4) === SIMD.float32x4(1, 2, 3, 4)
> 
> which probably doesn't work correctly yet.
> 
> https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js
> File test/mjsunit/messages.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/1/test/mjsunit/messages.js#ne...
> test/mjsunit/messages.js:328: 1 + SIMD.Float32x4(1, 2, 3, 4);
> Typo: SIMD.float32x4 (you got a TypeError for the wrong reason)

[Dan Ehrenberg, 2015-07-07 17:18:58]

littledan@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2015-07-07 17:18:59]

https://codereview.chromium.org/1219943002/diff/120001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/simd.js (right):

https://codereview.chromium.org/1219943002/diff/120001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:201: assertTrue(SIMD.float32x4(1, 2, 3, 4) ==
SIMD.float32x4(1, 2, 3, 4));
Unlike the other tests, this seems to be specific to float32x4. A way out could
be to use SIMD[type].splat(1) and SIMD[type].splat(NaN) (maybe with replaceLane
so you can have just one element different, if you care) so you don't have to
worry about the number of arguments.

https://codereview.chromium.org/1219943002/diff/120001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:261: return
Number.parseInt(typeName[typeName.length - 1]);
This won't work for int8x16. Maybe you could use SIMD[typeName].length
(#arguments that the function takes), or a regexp.

[rossberg, 2015-07-08 11:15:15]

Bill, can you please re-upload your patch sets such that they are separated from
rebases?

[rossberg, 2015-07-08 11:19:37]

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/code-stubs.h#newcode2753
src/code-stubs.h:2753: SIMD_TYPE,
On 2015/07/06 23:59:04, bbudge wrote:
> On 2015/07/02 13:35:43, rossberg wrote:
> > Nit: simply SIMD would seem more consistent with the other names.
> 
> How about SIMD_VALUE, since SIMD is more of an adjective than a noun?

Fine with me.

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1219943002/diff/1/src/contexts.h#newcode84
src/contexts.h:84: V(SIMD_OBJECT_INDEX, JSObject, simd_object)                  
               \
On 2015/07/06 23:59:04, bbudge wrote:
> On 2015/07/02 13:35:43, rossberg wrote:
> > Is this actually used?
> 
> Turns out it's needed for bootstrapper.cc:1329:
> native_context()->set_simd_object(*simd_object);

Well, that is just initialising the field. If it's never actually read, that's
likewise redundant.

[bbudge, 2015-07-08 14:40:05]

Patchset #7 (id:120001) has been deleted

[bbudge, 2015-07-08 14:40:24]

Patchset #6 (id:100001) has been deleted

[bbudge, 2015-07-08 14:40:46]

Patchset #5 (id:80001) has been deleted

[bbudge, 2015-07-08 14:41:04]

Patchset #4 (id:60001) has been deleted

[bbudge, 2015-07-08 14:41:20]

Patchset #3 (id:40001) has been deleted

[bbudge, 2015-07-08 14:41:44]

Patchset #2 (id:20001) has been deleted

[bbudge, 2015-07-08 14:42:00]

Patchset #2 (id:140001) has been deleted

[bbudge, 2015-07-08 16:56:27]

Patchset #2 (id:150001) has been deleted

[bbudge, 2015-07-08 17:06:53]

Patchset #2 (id:160001) has been deleted

[bbudge, 2015-07-08 20:51:02]

I separated out the rebase. Sorry, I didn't realize there was so much churn.

I also uploaded a reworked simd.js value-type test file in response to some
comments dehrenberg made, that were unfortunately lost when I re-uploaded.

PTAL

[bbudge, 2015-07-08 21:38:42]

https://codereview.chromium.org/1219943002/diff/170001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/simd.js (right):

https://codereview.chromium.org/1219943002/diff/170001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:207: assertFalse(SIMD.float32x4(1, 2, 3, NaN) ===
SIMD.float32x4(1, 2, 3, NaN));
These tests are changed to work for other types in the next patch.

[Dan Ehrenberg, 2015-07-09 00:23:18]

https://codereview.chromium.org/1219943002/diff/230001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1219943002/diff/230001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1317: {
Looks like this code runs regardless of --harmony_simd

[bbudge, 2015-07-09 01:11:32]

https://codereview.chromium.org/1219943002/diff/230001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1219943002/diff/230001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1317: {
On 2015/07/09 00:23:17, Dan Ehrenberg wrote:
> Looks like this code runs regardless of --harmony_simd

Fixed.

[martyn.capewell, 2015-07-09 10:48:06]

martyn.capewell@arm.com changed reviewers:
+ martyn.capewell@arm.com

[martyn.capewell, 2015-07-09 10:48:08]

https://codereview.chromium.org/1219943002/diff/250001/src/arm64/full-codegen...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/1219943002/diff/250001/src/arm64/full-codegen...
src/arm64/full-codegen-arm64.cc:4894:
ASM_LOCATION("FullCodeGenerator::EmitLiteralCompareTypeof symbol_string");
DBC: s/symbol_string/float32x4_string/

[rossberg, 2015-07-10 11:38:20]

Looking almost good. Sorry I overlooked the missing SameValue[Zero] adaptation
earlier.

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/harmony/r...
File test/mjsunit/harmony/reflect-apply.js (right):

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/harmony/r...
test/mjsunit/harmony/reflect-apply.js:5: // Flags: --harmony-reflect
--harmony-simd
This must not be needed, see other comment.

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
File test/mjsunit/object-toprimitive.js (right):

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
test/mjsunit/object-toprimitive.js:28: // Flags: --harmony-simd
If this test does not use SIMDs then this flag should not be needed. If the test
nevertheless fails without setting the flag, then that's a (serious) bug. You
are probably missing places where you need to make code paths dependent on the
flag.

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js#newcode953
src/runtime.js:953: function SameValue(x, y) {
This also needs to be extended, see the SIMD spec. Affects Object.is on SIMD
values.

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js#newcode967
src/runtime.js:967: function SameValueZero(x, y) {
Likewise. Affects maps/sets over SIMD values as keys.

[bbudge, 2015-07-10 13:21:17]

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
File test/mjsunit/object-toprimitive.js (right):

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
test/mjsunit/object-toprimitive.js:28: // Flags: --harmony-simd
On 2015/07/10 11:38:20, rossberg wrote:
> If this test does not use SIMDs then this flag should not be needed. If the
test
> nevertheless fails without setting the flag, then that's a (serious) bug. You
> are probably missing places where you need to make code paths dependent on the
> flag.

I don't understand the comment. This test does add a test for SIMD.float32x4,
below. Do you think I should remove this test, or make it conditional on SIMD
being defined?

[rossberg, 2015-07-10 13:25:45]

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
File test/mjsunit/object-toprimitive.js (right):

https://codereview.chromium.org/1219943002/diff/210001/test/mjsunit/object-to...
test/mjsunit/object-toprimitive.js:28: // Flags: --harmony-simd
On 2015/07/10 13:21:17, bbudge wrote:
> On 2015/07/10 11:38:20, rossberg wrote:
> > If this test does not use SIMDs then this flag should not be needed. If the
> test
> > nevertheless fails without setting the flag, then that's a (serious) bug.
You
> > are probably missing places where you need to make code paths dependent on
the
> > flag.
> 
> I don't understand the comment. This test does add a test for SIMD.float32x4,
> below. Do you think I should remove this test, or make it conditional on SIMD
> being defined?

Ah, sorry, I somehow got lost in the diffs. Please ignore this comment and the
other one. :)

[bbudge, 2015-07-10 18:48:33]

Patchset #7 (id:270001) has been deleted

[bbudge, 2015-07-10 19:08:17]

Patchset #7 (id:290001) has been deleted

[bbudge, 2015-07-10 19:14:30]

Andreas, I noticed that tests/mjsunit/samevalue.js does not test SameValueZero,
so I modified the tests. It seems to belong in the same file, since they are so
similar.

Let me know if you'd like me to split this into its own CL. Otherwise, I'll
update the issue description to include this change.

PTAL

https://codereview.chromium.org/1219943002/diff/250001/src/arm64/full-codegen...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/1219943002/diff/250001/src/arm64/full-codegen...
src/arm64/full-codegen-arm64.cc:4894:
ASM_LOCATION("FullCodeGenerator::EmitLiteralCompareTypeof symbol_string");
On 2015/07/09 10:48:07, martyn.capewell wrote:
> DBC: s/symbol_string/float32x4_string/

Thanks for catching this. Fixed.

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js#newcode953
src/runtime.js:953: function SameValue(x, y) {
On 2015/07/10 11:38:20, rossberg wrote:
> This also needs to be extended, see the SIMD spec. Affects Object.is on SIMD
> values.

Done.

https://codereview.chromium.org/1219943002/diff/250001/src/runtime.js#newcode967
src/runtime.js:967: function SameValueZero(x, y) {
On 2015/07/10 11:38:20, rossberg wrote:
> Likewise. Affects maps/sets over SIMD values as keys.

Done.

[Dan Ehrenberg, 2015-07-10 20:59:22]

These new tests make a good addition

https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue.js
File test/mjsunit/samevalue.js (right):

https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue...
test/mjsunit/samevalue.js:36: var sameValueZero = natives.$sameValueZero;
SUggestion: Make a version of sameValueZero which also checks !sameValue.

[bbudge, 2015-07-10 21:40:48]

https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue.js
File test/mjsunit/samevalue.js (right):

https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue...
test/mjsunit/samevalue.js:36: var sameValueZero = natives.$sameValueZero;
On 2015/07/10 20:59:22, Dan Ehrenberg wrote:
> SUggestion: Make a version of sameValueZero which also checks !sameValue.

Good idea. Done.

[Dan Ehrenberg, 2015-07-10 23:32:44]

On 2015/07/10 21:40:48, bbudge wrote:
>
https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue.js
> File test/mjsunit/samevalue.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue...
> test/mjsunit/samevalue.js:36: var sameValueZero = natives.$sameValueZero;
> On 2015/07/10 20:59:22, Dan Ehrenberg wrote:
> > SUggestion: Make a version of sameValueZero which also checks !sameValue.
> 
> Good idea. Done.

Test changes look good to me.

[Dan Ehrenberg, 2015-07-10 23:32:47]

On 2015/07/10 21:40:48, bbudge wrote:
>
https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue.js
> File test/mjsunit/samevalue.js (right):
> 
>
https://codereview.chromium.org/1219943002/diff/330001/test/mjsunit/samevalue...
> test/mjsunit/samevalue.js:36: var sameValueZero = natives.$sameValueZero;
> On 2015/07/10 20:59:22, Dan Ehrenberg wrote:
> > SUggestion: Make a version of sameValueZero which also checks !sameValue.
> 
> Good idea. Done.

Test changes look good to me.

[rossberg, 2015-07-13 11:13:23]

Thanks for adding the sameValueZero tests, was missing indeed.

Almost there. My only other request is to separate out the SIMD tests from the
ES5 tests, and move them into new files in harmony/. It would also be good to
have a test that uses SIMD values as keys in actual maps or sets (and checks
they are rejected for weak maps), to make sure all that works correctly.

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js#newcode963
src/runtime.js:963: return %Float32x4SameValue(x, y) ? true : false;
The ?: seems redundant.

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js#newcode976
src/runtime.js:976: return %Float32x4SameValueZero(x, y) ? true : false;
Same here.

https://codereview.chromium.org/1219943002/diff/310001/test/mjsunit/samevalue.js
File test/mjsunit/samevalue.js (right):

https://codereview.chromium.org/1219943002/diff/310001/test/mjsunit/samevalue...
test/mjsunit/samevalue.js:131: assertTrue(sameValueBoth(SIMD.float32x4(+0, -0,
NaN, 1),
Perhaps don't have NaN in here, to be sure we get the correct result for the
correct reason in all cases.

[bbudge, 2015-07-13 23:10:39]

Moved SIMD tests out of es5 tests (samevalue.js, object-toprimitive.js) and into
the simd.js test file. Left Simd tests in reflect-apply.js, since it's also in
tests/harmony.

Added hash code calculation for Float32x4 values in object.cc.

Added Set/WeakSet and Map/WeakMap tests in simd.js.

PTAL

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js#newcode963
src/runtime.js:963: return %Float32x4SameValue(x, y) ? true : false;
On 2015/07/13 11:13:23, rossberg wrote:
> The ?: seems redundant.

Yep, I was trying to fix a weird problem and thought it might relate to return
type. Instead, I had left the declaration of this fn out of runtime.h. Forgot to
remove this.

Done.

https://codereview.chromium.org/1219943002/diff/310001/src/runtime.js#newcode976
src/runtime.js:976: return %Float32x4SameValueZero(x, y) ? true : false;
On 2015/07/13 11:13:23, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/1219943002/diff/310001/test/mjsunit/samevalue.js
File test/mjsunit/samevalue.js (right):

https://codereview.chromium.org/1219943002/diff/310001/test/mjsunit/samevalue...
test/mjsunit/samevalue.js:131: assertTrue(sameValueBoth(SIMD.float32x4(+0, -0,
NaN, 1),
On 2015/07/13 11:13:23, rossberg wrote:
> Perhaps don't have NaN in here, to be sure we get the correct result for the
> correct reason in all cases.

I'll separate out the tests. BTW I'll rewrite these tests once we have
replaceLane defined, to be exhaustive.

https://codereview.chromium.org/1219943002/diff/350001/test/mjsunit/object-to...
File test/mjsunit/object-toprimitive.js (right):

https://codereview.chromium.org/1219943002/diff/350001/test/mjsunit/object-to...
test/mjsunit/object-toprimitive.js:110: assertThrows('Number(SIMD.float32x4(1,
2, 3, 4))', TypeError);
I'll just delete this test - it's covered by TestToNumber in
tests/mjsunit/harmony/simd.js.

https://codereview.chromium.org/1219943002/diff/350001/test/mjsunit/samevalue.js
File test/mjsunit/samevalue.js (right):

https://codereview.chromium.org/1219943002/diff/350001/test/mjsunit/samevalue...
test/mjsunit/samevalue.js:140: SIMD.float32x4(+0, +0, NaN, 1)));
I'll move these to tests/mjsunit/harmony/simd.js.

[rossberg, 2015-07-14 06:43:47]

LGTM, just one nit:

> Left Simd tests in reflect-apply.js, since it's also in tests/harmony.

Yes. However, the reflect tests will eventually be promoted to es6/, while SIMD
would move somewhere else. (Maybe this is not the ideal structure moving
forward, but let's keep it for now.)

[bbudge, 2015-07-15 18:32:17]

I moved the SIMD reflect-apply tests to simd.js. I agree it's better to keep
these with the other SIMD tests for now.

I wasn't able to add JS tests for Object::SameValue/Zero via Object.freeze
because everything seems to go through the intrinsic SameValue (see v8natives.js
DefineObjectProperty.)

[bbudge, 2015-07-15 19:14:12]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2015-07-15 19:14:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1219943002/#ps450001
(title: "Remove SIMD_OBJECT_TYPE that wasn't deleted.")

[commit-bot: I haz the power, 2015-07-15 19:14:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219943002/450001

[commit-bot: I haz the power, 2015-07-15 19:17:00]

Committed patchset #14 (id:450001)

[commit-bot: I haz the power, 2015-07-15 19:17:19]

Patchset 14 (id:??) landed as
https://crrev.com/e5ed3bee99807c502fa7d7a367ec401e16d3f773
Cr-Commit-Position: refs/heads/master@{#29689}

[Michael Hablich, 2015-07-16 12:35:17]

A revert of this CL (patchset #14 id:450001) has been created in
https://codereview.chromium.org/1241533004/ by hablich@chromium.org.

The reason for reverting is: Seems to brake the latest roll into Chromium:
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...

.

[bbudge, 2015-07-16 17:27:46]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2015-07-16 17:27:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1219943002/#ps470001
(title: "Back out changes to include/v8.h")

[commit-bot: I haz the power, 2015-07-16 17:28:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219943002/470001

[commit-bot: I haz the power, 2015-07-16 19:43:28]

Committed patchset #15 (id:470001)

[commit-bot: I haz the power, 2015-07-16 19:43:43]

Patchset 15 (id:??) landed as
https://crrev.com/61130584270ebffdfc43c4ab802c6b0b75f1504b
Cr-Commit-Position: refs/heads/master@{#29712}