Mostly looking good, but I wonder about the cost of the IS_SIMD_VALUE
predicate.
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js
File src/harmony-simd.js (right):
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js#newcode35
src/harmony-simd.js:35: return %CreateFloat32x4(TO_NUMBER_INLINE(c0),
TO_NUMBER_INLINE(c1), TO_NUMBER_INLINE(c2), TO_NUMBER_INLINE(c3));
Nit: line length
https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js#newcode90
src/harmony-simd.js:90:
//-------------------------------------------------------------------
Would it be possible to avoid all the repetition by using a macro? (See
e.g. typedarray.js)
https://codereview.chromium.org/1250733005/diff/210001/src/heap/heap.cc
File src/heap/heap.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/src/heap/heap.cc#newcode3119
src/heap/heap.cc:3119: AllocationResult Heap::AllocateFloat32x4(float
lanes[4],
Macrofication might help here, too.
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py
File src/macros.py (right):
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py#newcode136
src/macros.py:136: macro IS_SIMD_VALUE(arg) = (IS_FLOAT32X4(arg)
|| IS_INT32X4(arg) || IS_BOOL32X4(arg) || IS_INT16X8(arg) ||
IS_BOOL16X8(arg) || IS_INT8X16(arg) || IS_BOOL8X16(arg));
Hm, this seems rather expensive. It is used on some potentially
performance-relevant paths, so there should probably be a %_IsSimdValue
intrinsic to make it efficient. I'm fine with doing that in a follow-up
CL, but better be prepared that landing the CL as is might affect some
benchmarks.
https://codereview.chromium.org/1250733005/diff/210001/src/macros.py#newcode137
src/macros.py:137: macro IS_SIMD_VALUE_WRAPPER(arg) =
(IS_FLOAT32X4_WRAPPER(arg) || IS_INT32X4_WRAPPER(arg) ||
IS_BOOL32X4_WRAPPER(arg) || IS_INT16X8_WRAPPER(arg) ||
IS_BOOL16X8_WRAPPER(arg) || IS_INT8X16_WRAPPER(arg) ||
IS_BOOL8X16_WRAPPER(arg));
Is this actually used?
https://codereview.chromium.org/1250733005/diff/210001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/src/objects.cc#newcode102
src/objects.cc:102: return MaybeHandle<JSReceiver>();
Nit: the return here is redundant.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js
File src/runtime.js (right):
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode136
src/runtime.js:136: } else if (IS_FLOAT32X4(x)) {
Guard these by a (cheaper) IS_SIMD_VALUE test.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode189
src/runtime.js:189: if (IS_FLOAT32X4(this) && IS_FLOAT32X4(x))
Same here.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode932
src/runtime.js:932: if (IS_FLOAT32X4(x)) return %Float32x4SameValue(x,
y);
And here.
https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode949
src/runtime.js:949: if (IS_FLOAT32X4(x)) return
%Float32x4SameValueZero(x, y);
And here.
https://codereview.chromium.org/1250733005/diff/210001/test/cctest/test-simd.cc
File test/cctest/test-simd.cc (right):
https://codereview.chromium.org/1250733005/diff/210001/test/cctest/test-simd.cc#newcode14
test/cctest/test-simd.cc:14: TEST(SameValue) {
Any reason not to test the other types, too?
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js
File test/mjsunit/harmony/simd.js (right):
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode277
test/mjsunit/harmony/simd.js:277: // SIMD values should not be equal to
any other kind of object.
Can you add tests involving two different SIMD types?
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode324
test/mjsunit/harmony/simd.js:324: function TestSameValue(type, lanes) {
Same here.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode371
test/mjsunit/harmony/simd.js:371: var a = createInstance(type), b =
createInstance(type);
Again it would be good to check the matrix of SIMD types, and also
include a couple of non-SIMD values on either side.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode423
test/mjsunit/harmony/simd.js:423: test(set, instance);
This now only tests for a zeroed value, which seems a bit limited.
https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/simd.js#newcode451
test/mjsunit/harmony/simd.js:451: test(map, instance, {});
Similarly here.
https://codereview.chromium.org/1250733005/
--
--
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.
