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.cc#newcode153
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#newcode634
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#newcode242
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#newcode245
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#newcode254
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.js#newcode46
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.js#newcode68
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.js#newcode158
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#newcode328
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.
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.
