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.cc#newcode153
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#newcode634
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#newcode242
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#newcode245
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#newcode254
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.js#newcode46
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.js#newcode68
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.js#newcode158
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#newcode328
test/mjsunit/messages.js:328: 1 + SIMD.Float32x4(1, 2, 3, 4);
Typo: SIMD.float32x4 (you got a TypeError for the wrong reason)
https://codereview.chromium.org/1219943002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
