SIMD.js Add the other SIMD Phase 1 types.

SIMD.js Add the other SIMD Phase 1 types.

Adds Int32x4, Bool32x4, Int16x8, Bool16x8, Int8x16, Bool8x16.
Adds Simd128Value base heap object class.
Changes heap/factory construction pattern to use arrays.
Adds replaceLane functions to facilitate testing.

NOPRESUBMIT=true
(presubmit checks erroneously interpret array declaration in macro definition as variable size array.)

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

[bbudge, 2015-07-21 21:45:59]

Patchset #2 (id:20001) has been deleted

[bbudge, 2015-07-24 19:17:53]

Patchset #1 (id:1) has been deleted

[bbudge, 2015-07-24 19:17:59]

Patchset #1 (id:40001) has been deleted

[bbudge, 2015-07-24 19:18:05]

Patchset #1 (id:50001) has been deleted

[bbudge, 2015-07-25 14:47:34]

Patchset #1 (id:70001) has been deleted

[bbudge, 2015-07-25 15:17:48]

Patchset #2 (id:110001) has been deleted

[bbudge, 2015-07-26 13:07:25]

Patchset #2 (id:130001) has been deleted

[bbudge, 2015-07-26 13:27:56]

Patchset #2 (id:150001) has been deleted

[bbudge, 2015-07-28 16:57:25]

Patchset #1 (id:90001) has been deleted

[bbudge, 2015-07-28 16:57:37]

Patchset #1 (id:170001) has been deleted

[bbudge, 2015-07-28 17:08:41]

Looks like the problem on Windows (DCHECK failing) went away.
PTAL

[bbudge, 2015-07-28 17:09:10]

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

[rossberg, 2015-07-29 14:37:56]

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#ne...
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#ne...
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#newco...
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....
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/s...
File test/mjsunit/harmony/simd.js (right):

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
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/s...
test/mjsunit/harmony/simd.js:324: function TestSameValue(type, lanes) {
Same here.

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
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/s...
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/s...
test/mjsunit/harmony/simd.js:451: test(map, instance, {});
Similarly here.

[bbudge, 2015-07-30 13:46:59]

I macro-ized SIMD types in C++ and JS to reduce repetition.
objects.h defines SIMD128_TYPES in a V8 style, and uses it to define the object
types. I moved some methods off Float32x4, which belonged on the Object::
methods any way.
heap and bootstrapper now use SIMD128_TYPES.

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#ne...
src/harmony-simd.js:35: return %CreateFloat32x4(TO_NUMBER_INLINE(c0),
TO_NUMBER_INLINE(c1), TO_NUMBER_INLINE(c2), TO_NUMBER_INLINE(c3));
On 2015/07/29 14:37:55, rossberg wrote:
> Nit: line length

Done.

https://codereview.chromium.org/1250733005/diff/210001/src/harmony-simd.js#ne...
src/harmony-simd.js:90:
//-------------------------------------------------------------------
On 2015/07/29 14:37:55, rossberg wrote:
> Would it be possible to avoid all the repetition by using a macro? (See e.g.
> typedarray.js)

Macros in JS? I didn't know! Done.

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#newco...
src/heap/heap.cc:3119: AllocationResult Heap::AllocateFloat32x4(float lanes[4],
On 2015/07/29 14:37:55, rossberg wrote:
> Macrofication might help here, too.

Definitely. Done.

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));
On 2015/07/29 14:37:55, rossberg wrote:
> 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.

I don't like those perf bugs, so I'll use your suggestion now. Done.

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));
On 2015/07/29 14:37:55, rossberg wrote:
> Is this actually used?

Nope. Deleted.

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>();
On 2015/07/29 14:37:55, rossberg wrote:
> Nit: the return here is redundant.

Done.

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#newcode106
src/runtime.js:106: if (IS_SYMBOL(y) || IS_SIMD_VALUE(y)) return 1;  // not
equal
I moved these primitive checks which always return "not equal" to the end of
their blocks, since they shouldn't be encountered in real or benchmark code. 
WDYT?

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode116
src/runtime.js:116: if (IS_SYMBOL(y) || IS_SIMD_VALUE(y)) return 1;  // not
equal
Made this %IsSimdValue call and moved it to the end of this block with the
IS_SYMBOL case.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode136
src/runtime.js:136: } else if (IS_FLOAT32X4(x)) {
On 2015/07/29 14:37:55, rossberg wrote:
> Guard these by a (cheaper) IS_SIMD_VALUE test.

Done. Using %IsSimdValue intrinsic. Also, moved all the type checking into a
SimdEquals intrinsic.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode170
src/runtime.js:170: if (IS_SYMBOL(y) || IS_SIMD_VALUE(y)) return 1;  // not
equal
Moved this to after the IS_BOOLEAN case.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode189
src/runtime.js:189: if (IS_FLOAT32X4(this) && IS_FLOAT32X4(x))
On 2015/07/29 14:37:55, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode884
src/runtime.js:884: if (IS_BOOL8X16(x)) return %NewBool8x16Wrapper(x);
Did the same thing here too, with the %IsSimdValue and a %SimdToObject
intrinsic.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode932
src/runtime.js:932: if (IS_FLOAT32X4(x)) return %Float32x4SameValue(x, y);
On 2015/07/29 14:37:55, rossberg wrote:
> And here.

Done.

https://codereview.chromium.org/1250733005/diff/210001/src/runtime.js#newcode949
src/runtime.js:949: if (IS_FLOAT32X4(x)) return %Float32x4SameValueZero(x, y);
On 2015/07/29 14:37:56, rossberg wrote:
> And here.

Done.

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....
test/cctest/test-simd.cc:14: TEST(SameValue) {
On 2015/07/29 14:37:56, rossberg wrote:
> Any reason not to test the other types, too?

I just forgot about these. Done.

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

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:277: // SIMD values should not be equal to any
other kind of object.
On 2015/07/29 14:37:56, rossberg wrote:
> Can you add tests involving two different SIMD types?

Good idea. Done.

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:324: function TestSameValue(type, lanes) {
On 2015/07/29 14:37:56, rossberg wrote:
> Same here.

Done.

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:371: var a = createInstance(type), b =
createInstance(type);
On 2015/07/29 14:37:56, rossberg wrote:
> Again it would be good to check the matrix of SIMD types, and also include a
> couple of non-SIMD values on either side.

Done.

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:423: test(set, instance);
On 2015/07/29 14:37:56, rossberg wrote:
> This now only tests for a zeroed value, which seems a bit limited.

I changed createInstance to create slightly more interesting values.

https://codereview.chromium.org/1250733005/diff/210001/test/mjsunit/harmony/s...
test/mjsunit/harmony/simd.js:451: test(map, instance, {});
On 2015/07/29 14:37:56, rossberg wrote:
> Similarly here.

Done.

[rossberg, 2015-07-30 15:12:24]

LGTM modulo comments. Let's see how this version performs. We can always roll
back.

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));
On 2015/07/30 13:46:58, bbudge wrote:
> On 2015/07/29 14:37:55, rossberg wrote:
> > 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.
> 
> I don't like those perf bugs, so I'll use your suggestion now. Done.

Ah, note the difference between assembly intrinsics (%_XY) and C intrinsics
(%XY). The former are recognised as distinguished AST nodes for which code
generators generate inline assembly directly. The latter create runtime calls,
which are expensive. I actually had the former in mind, but that involves some
work. I doubt the latter will perform much better than the multi-disjunction.
But I guess we'll find out. :)

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#newcode106
src/runtime.js:106: if (IS_SYMBOL(y) || IS_SIMD_VALUE(y)) return 1;  // not
equal
On 2015/07/30 13:46:58, bbudge wrote:
> I moved these primitive checks which always return "not equal" to the end of
> their blocks, since they shouldn't be encountered in real or benchmark code. 
> WDYT?

Fine with me.

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
I'm surprised this actually works! See above, you should have written
%IsSimdValue here.

https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-si...
File src/runtime/runtime-simd.cc (right):

https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-si...
src/runtime/runtime-simd.cc:167: return Smi::FromInt(result ? EQUAL :
NOT_EQUAL);
It is a bit strange to return an int here and Booleans for the other functions.

[adamk, 2015-07-30 15:34:57]

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

[adamk, 2015-07-30 15:34:58]

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
On 2015/07/30 15:12:24, rossberg wrote:
> I'm surprised this actually works! See above, you should have written
> %IsSimdValue here.

Drive-by explanation of why this works: since a recent refactor (by Sven I think
it was), the "%_" form works for all runtime calls, and the backends fall back
to the runtime if they don't have an implementation of a given intrinsic.

[rossberg, 2015-07-30 15:50:39]

On 2015/07/30 15:34:58, adamk wrote:
> https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> File src/macros.py (right):
> 
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
> On 2015/07/30 15:12:24, rossberg wrote:
> > I'm surprised this actually works! See above, you should have written
> > %IsSimdValue here.
> 
> Drive-by explanation of why this works: since a recent refactor (by Sven I
think
> it was), the "%_" form works for all runtime calls, and the backends fall back
> to the runtime if they don't have an implementation of a given intrinsic.

Hm, was there a particular motivation for that change? Seems to make intent less
clear and accidents go unnoticed.

[bbudge, 2015-07-30 16:21:27]

On 2015/07/30 15:50:39, rossberg wrote:
> On 2015/07/30 15:34:58, adamk wrote:
> > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> > File src/macros.py (right):
> > 
> >
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> > src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
> > On 2015/07/30 15:12:24, rossberg wrote:
> > > I'm surprised this actually works! See above, you should have written
> > > %IsSimdValue here.
> > 
> > Drive-by explanation of why this works: since a recent refactor (by Sven I
> think
> > it was), the "%_" form works for all runtime calls, and the backends fall
back
> > to the runtime if they don't have an implementation of a given intrinsic.
> 
> Hm, was there a particular motivation for that change? Seems to make intent
less
> clear and accidents go unnoticed.

I'd be happy to implement the assembly intrinsic. Is there an example or some
documentation for the extra work that needs to be done?

[adamk, 2015-07-30 17:00:43]

On 2015/07/30 15:50:39, rossberg wrote:
> On 2015/07/30 15:34:58, adamk wrote:
> > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> > File src/macros.py (right):
> > 
> >
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> > src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
> > On 2015/07/30 15:12:24, rossberg wrote:
> > > I'm surprised this actually works! See above, you should have written
> > > %IsSimdValue here.
> > 
> > Drive-by explanation of why this works: since a recent refactor (by Sven I
> think
> > it was), the "%_" form works for all runtime calls, and the backends fall
back
> > to the runtime if they don't have an implementation of a given intrinsic.
> 
> Hm, was there a particular motivation for that change? Seems to make intent
less
> clear and accidents go unnoticed.

We already were in a weird place since we started adding intrinsics that were
only implemented in Crankshaft (and runtime). With the introduction of
TurboFan-backed intrinsics, this got even weirder. I think the motivation makes
sense, though I agree it's annoying that you can't tell just by looking at a
callsite whether it'll be inlined.

[rossberg, 2015-07-30 17:07:22]

On 2015/07/30 16:21:27, bbudge wrote:
> On 2015/07/30 15:50:39, rossberg wrote:
> > On 2015/07/30 15:34:58, adamk wrote:
> > > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> > > File src/macros.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> > > src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
> > > On 2015/07/30 15:12:24, rossberg wrote:
> > > > I'm surprised this actually works! See above, you should have written
> > > > %IsSimdValue here.
> > > 
> > > Drive-by explanation of why this works: since a recent refactor (by Sven I
> > think
> > > it was), the "%_" form works for all runtime calls, and the backends fall
> back
> > > to the runtime if they don't have an implementation of a given intrinsic.
> > 
> > Hm, was there a particular motivation for that change? Seems to make intent
> less
> > clear and accidents go unnoticed.
> 
> I'd be happy to implement the assembly intrinsic. Is there an example or some
> documentation for the extra work that needs to be done?

Documentation? For V8? Are you sure you have already lowered your expectations
about V8 far enough? :)

But it should work to follow the model of other such intrinsics, e.g. ClassOf.
The main annoyance is that you'll have to implement it for all the gazillion
compilers times architectures.

[bbudge, 2015-07-30 17:13:15]

On 2015/07/30 17:07:22, rossberg wrote:
> On 2015/07/30 16:21:27, bbudge wrote:
> > On 2015/07/30 15:50:39, rossberg wrote:
> > > On 2015/07/30 15:34:58, adamk wrote:
> > > > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> > > > File src/macros.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> > > > src/macros.py:101: macro IS_SIMD_VALUE(arg)        =
(%_IsSimdValue(arg));
> > > > On 2015/07/30 15:12:24, rossberg wrote:
> > > > > I'm surprised this actually works! See above, you should have written
> > > > > %IsSimdValue here.
> > > > 
> > > > Drive-by explanation of why this works: since a recent refactor (by Sven
I
> > > think
> > > > it was), the "%_" form works for all runtime calls, and the backends
fall
> > back
> > > > to the runtime if they don't have an implementation of a given
intrinsic.
> > > 
> > > Hm, was there a particular motivation for that change? Seems to make
intent
> > less
> > > clear and accidents go unnoticed.
> > 
> > I'd be happy to implement the assembly intrinsic. Is there an example or
some
> > documentation for the extra work that needs to be done?
> 
> Documentation? For V8? Are you sure you have already lowered your expectations
> about V8 far enough? :)
> 
> But it should work to follow the model of other such intrinsics, e.g. ClassOf.
> The main annoyance is that you'll have to implement it for all the gazillion
> compilers times architectures.

I should have learned by now to never say "happy to do the work" in V8 until I
know just how much work there is. : )  Thanks for your review BTW.

[bbudge, 2015-07-30 17:21:03]

On 2015/07/30 17:07:22, rossberg wrote:
> On 2015/07/30 16:21:27, bbudge wrote:
> > On 2015/07/30 15:50:39, rossberg wrote:
> > > On 2015/07/30 15:34:58, adamk wrote:
> > > > https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
> > > > File src/macros.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
> > > > src/macros.py:101: macro IS_SIMD_VALUE(arg)        =
(%_IsSimdValue(arg));
> > > > On 2015/07/30 15:12:24, rossberg wrote:
> > > > > I'm surprised this actually works! See above, you should have written
> > > > > %IsSimdValue here.
> > > > 
> > > > Drive-by explanation of why this works: since a recent refactor (by Sven
I
> > > think
> > > > it was), the "%_" form works for all runtime calls, and the backends
fall
> > back
> > > > to the runtime if they don't have an implementation of a given
intrinsic.
> > > 
> > > Hm, was there a particular motivation for that change? Seems to make
intent
> > less
> > > clear and accidents go unnoticed.
> > 
> > I'd be happy to implement the assembly intrinsic. Is there an example or
some
> > documentation for the extra work that needs to be done?
> 
> Documentation? For V8? Are you sure you have already lowered your expectations
> about V8 far enough? :)
> 
> But it should work to follow the model of other such intrinsics, e.g. ClassOf.
> The main annoyance is that you'll have to implement it for all the gazillion
> compilers times architectures.

_IsSpecObject seems to be a good model (and more appropriate for SIMD stuff,
which won't be optimized for hydrogen/lithium.) It looks like something I could
add to this patch. Anything I'm missing?

[bbudge, 2015-07-30 21:05:27]

Patchset #5 (id:270001) has been deleted

[bbudge, 2015-07-30 21:42:52]

Patchset #5 (id:290001) has been deleted

[bbudge, 2015-07-30 22:28:23]

Patchset #6 (id:330001) has been deleted

[bbudge, 2015-07-30 22:36:05]

Patchset #6 (id:350001) has been deleted

[bbudge, 2015-07-30 22:42:47]

Patchset #6 (id:370001) has been deleted

[bbudge, 2015-07-30 23:27:01]

I made _IsSimdObject an assembly intrinsic, at least for fullcodegen.

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/1250733005/diff/230001/src/macros.py#newcode101
src/macros.py:101: macro IS_SIMD_VALUE(arg)        = (%_IsSimdValue(arg));
On 2015/07/30 15:34:58, adamk wrote:
> On 2015/07/30 15:12:24, rossberg wrote:
> > I'm surprised this actually works! See above, you should have written
> > %IsSimdValue here.
> 
> Drive-by explanation of why this works: since a recent refactor (by Sven I
think
> it was), the "%_" form works for all runtime calls, and the backends fall back
> to the runtime if they don't have an implementation of a given intrinsic.

Added the intrinsic, at least for fullcodegen. I tried adding it to hydrogen but
hit this DCHECK:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/hydrogen-in...

which leads me to believe it's not possible.

https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-si...
File src/runtime/runtime-simd.cc (right):

https://codereview.chromium.org/1250733005/diff/230001/src/runtime/runtime-si...
src/runtime/runtime-simd.cc:167: return Smi::FromInt(result ? EQUAL :
NOT_EQUAL);
On 2015/07/30 15:12:24, rossberg wrote:
> It is a bit strange to return an int here and Booleans for the other
functions.

It's driven by the call site, in EQUALS (runtime.js) where we need to return an
int. It matches %NumberEquals and it's relatives. This way I don't have to do
the %SimdEquals ? 0 : 1 in JS.

[bbudge, 2015-08-03 11:42:13]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2015-08-03 11:42:13]

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/1250733005/#ps450001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-03 11:42:25]

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

[commit-bot: I haz the power, 2015-08-03 11:45:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-03 11:45:57]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4632)

[bbudge, 2015-08-03 12:22:35]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2015-08-03 12:22:35]

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/1250733005/#ps470001
(title: "Fix harness-adapt.js.")

[commit-bot: I haz the power, 2015-08-03 12:22:44]

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

[commit-bot: I haz the power, 2015-08-03 12:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-03 12:31:20]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8332)

[bbudge, 2015-08-03 12:38:19]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2015-08-03 12:38:20]

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/1250733005/#ps490001
(title: "Don't run downloaded SIMD value type tests.")

[commit-bot: I haz the power, 2015-08-03 12:38:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1250733005/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1250733005/490001

[commit-bot: I haz the power, 2015-08-03 13:02:45]

Committed patchset #11 (id:490001)

[commit-bot: I haz the power, 2015-08-03 13:03:06]

Patchset 11 (id:??) landed as
https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
Cr-Commit-Position: refs/heads/master@{#29974}

[Yang, 2015-08-03 13:54:42]

On 2015/08/03 13:03:06, commit-bot: I haz the power wrote:
> Patchset 11 (id:??) landed as
> https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
> Cr-Commit-Position: refs/heads/master@{#29974}

Well by simply marking NOPRESUBMIT=true, other test bots in the CQ complain
about the same issue
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4638/st...

[titzer, 2015-08-03 14:10:34]

On 2015/08/03 13:54:42, Yang wrote:
> On 2015/08/03 13:03:06, commit-bot: I haz the power wrote:
> > Patchset 11 (id:??) landed as
> > https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
> > Cr-Commit-Position: refs/heads/master@{#29974}
> 
> Well by simply marking NOPRESUBMIT=true, other test bots in the CQ complain
> about the same issue
>
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4638/st...

Yes, please make sure the CQ runs all of the trybots, including v8-presubmit.

This CL changed a lot of compiler code in both Crankshaft and fullcode; it's
normal to loop in a compiler team person or two for the review in such cases.

[bbudge, 2015-08-03 14:31:31]

On 2015/08/03 14:10:34, titzer wrote:
> On 2015/08/03 13:54:42, Yang wrote:
> > On 2015/08/03 13:03:06, commit-bot: I haz the power wrote:
> > > Patchset 11 (id:??) landed as
> > > https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
> > > Cr-Commit-Position: refs/heads/master@{#29974}
> > 
> > Well by simply marking NOPRESUBMIT=true, other test bots in the CQ complain
> > about the same issue
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4638/st...
> 
> Yes, please make sure the CQ runs all of the trybots, including v8-presubmit.
> 
> This CL changed a lot of compiler code in both Crankshaft and fullcode; it's
> normal to loop in a compiler team person or two for the review in such cases.

Thanks for the fix Ben. Sorry for any trouble this may have caused. I would
definitely appreciate a compiler person reviewing the change. If you prefer, we
can revert this and review it properly.

[rossberg, 2015-08-04 12:58:31]

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

[rossberg, 2015-08-04 13:03:46]

On 2015/08/03 14:31:31, bbudge wrote:
> On 2015/08/03 14:10:34, titzer wrote:
> > On 2015/08/03 13:54:42, Yang wrote:
> > > On 2015/08/03 13:03:06, commit-bot: I haz the power wrote:
> > > > Patchset 11 (id:??) landed as
> > > > https://crrev.com/7b9670b63b486ba3b6f8a569552d307282dbccfd
> > > > Cr-Commit-Position: refs/heads/master@{#29974}
> > > 
> > > Well by simply marking NOPRESUBMIT=true, other test bots in the CQ
complain
> > > about the same issue
> > >
> >
>
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4638/st...
> > 
> > Yes, please make sure the CQ runs all of the trybots, including
v8-presubmit.
> > 
> > This CL changed a lot of compiler code in both Crankshaft and fullcode; it's
> > normal to loop in a compiler team person or two for the review in such
cases.
> 
> Thanks for the fix Ben. Sorry for any trouble this may have caused. I would
> definitely appreciate a compiler person reviewing the change. If you prefer,
we
> can revert this and review it properly.

No point in reverting, I think -- you can address comments in a follow-up CLs.
But in general, when you change hundreds of lines of code after an LGTM, it's
better to wait for another round of review before landing. ;)

Added Ben as an additional reviewer for the compiler-related changes in this CL.

[rossberg, 2015-08-04 13:09:17]

https://codereview.chromium.org/1250733005/diff/310001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/1250733005/diff/310001/src/macros.py#newcode101
src/macros.py:101: macro IS_SIMD_OBJECT(arg)       = (%_IsSimdObject(arg));
Please revert this renaming. SIMD values are not objects, so it is misleading
here (even though we conflate terminology on lower levels).

[bbudge, 2015-08-04 14:14:31]

https://codereview.chromium.org/1250733005/diff/310001/src/macros.py
File src/macros.py (right):

https://codereview.chromium.org/1250733005/diff/310001/src/macros.py#newcode101
src/macros.py:101: macro IS_SIMD_OBJECT(arg)       = (%_IsSimdObject(arg));
On 2015/08/04 13:09:16, rossberg wrote:
> Please revert this renaming. SIMD values are not objects, so it is misleading
> here (even though we conflate terminology on lower levels).

I saw all the other IsFooObject functions when I was doing the intrinsic but
you're right.
https://codereview.chromium.org/1253103006/