Remove ExternalArray, derived types, and element kinds

Remove ExternalArray, derived types, and element kinds

BUG=v8:3996
R=jarin@chromium.org, mvstanton@chromium.org, bmeurer@chromium.org
LOG=y

Committed: https://crrev.com/607ef7c6009a24ebf195b4cab7b0b436c5afd21c
Cr-Commit-Position: refs/heads/master@{#29872}

[jochen (gone - plz use gerrit), 2015-07-24 10:08:25]

jochen@chromium.org changed reviewers:
+ bmeurer@chromium.org, mvstanton@chromium.org

[jochen (gone - plz use gerrit), 2015-07-24 10:08:36]

PTAL

[jochen (gone - plz use gerrit), 2015-07-24 10:24:29]

jochen@chromium.org changed reviewers:
+ hpayer@chromium.org

[jochen (gone - plz use gerrit), 2015-07-24 10:24:29]

bmeurer@/jarin@ - ptal at everything, esp. crankshaft & objects.cc

mvstanton@ - ptal at ic/

hpayer@ - ptal at heap/

[jochen (gone - plz use gerrit), 2015-07-24 12:16:17]

fix for the failing mjsunit test here:
https://codereview.chromium.org/1252333002

[Hannes Payer (out of office), 2015-07-24 13:40:59]

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3916: int size = FixedTypedArrayBase::kHeaderSize;
Can we have a kSize?

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3924: result->set_map(MapForFixedTypedArray(array_type));
Why not set_map_no_write_barrier?

[jochen (gone - plz use gerrit), 2015-07-24 16:04:36]

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3916: int size = FixedTypedArrayBase::kHeaderSize;
On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> Can we have a kSize?

that's misleading, because a fixed array can be on heap at which point the size
depends on it's length

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3924: result->set_map(MapForFixedTypedArray(array_type));
On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> Why not set_map_no_write_barrier?

copied from AllocateFixedTypedArray below

[jochen (gone - plz use gerrit), 2015-07-24 16:04:43]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-24 16:04:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254623002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254623002/20001

[commit-bot: I haz the power, 2015-07-24 16:06:05]

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

[commit-bot: I haz the power, 2015-07-24 16:06:06]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/7041)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8081)

[Hannes Payer (out of office), 2015-07-27 08:40:05]

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3916: int size = FixedTypedArrayBase::kHeaderSize;
On 2015/07/24 16:04:36, jochen wrote:
> On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> > Can we have a kSize?
> 
> that's misleading, because a fixed array can be on heap at which point the
size
> depends on it's length

Both sides are misleading. I do not have a strong preference.

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3924: result->set_map(MapForFixedTypedArray(array_type));
On 2015/07/24 16:04:36, jochen wrote:
> On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> > Why not set_map_no_write_barrier?
> 
> copied from AllocateFixedTypedArray below

This is part of the roots_ array. No need for a write barrier.

[mvstanton, 2015-07-27 08:44:42]

ic.cc, the ic and handler compilers, also code-stubs-hydrogen.cc LGTM.

[jochen (gone - plz use gerrit), 2015-07-27 08:46:16]

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3916: int size = FixedTypedArrayBase::kHeaderSize;
On 2015/07/27 at 08:40:05, Hannes Payer wrote:
> On 2015/07/24 16:04:36, jochen wrote:
> > On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> > > Can we have a kSize?
> > 
> > that's misleading, because a fixed array can be on heap at which point the
size
> > depends on it's length
> 
> Both sides are misleading. I do not have a strong preference.

I'll keep kHeaderSize then :)

https://codereview.chromium.org/1254623002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3924: result->set_map(MapForFixedTypedArray(array_type));
On 2015/07/27 at 08:40:05, Hannes Payer wrote:
> On 2015/07/24 16:04:36, jochen wrote:
> > On 2015/07/24 at 13:40:59, Hannes Payer wrote:
> > > Why not set_map_no_write_barrier?
> > 
> > copied from AllocateFixedTypedArray below
> 
> This is part of the roots_ array. No need for a write barrier.

fixed both

[Hannes Payer (out of office), 2015-07-27 09:02:02]

GC lgtm

[Benedikt Meurer, 2015-07-27 12:28:01]

LGTM with bunch of nits.

https://codereview.chromium.org/1254623002/diff/60001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.cc:84: FieldAccess
AccessBuilder::ForExternalArrayPointer() {
Nit: Please remove this method; it is unused anyway, and misleading.

https://codereview.chromium.org/1254623002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/factory.cc#newcode886
src/factory.cc:886: Handle<FixedTypedArrayBase> Factory::NewExternalArray(
Nit: Can we rename this method to NewFixedTypedArray (WithExternalPointer) ?

https://codereview.chromium.org/1254623002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3948: AllocationResult Heap::AllocateExternalArray(int length,
Nit: Same here, rename to AllocateFixedTypedArray ?

https://codereview.chromium.org/1254623002/diff/60001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1254623002/diff/60001/src/hydrogen-instructio...
src/hydrogen-instructions.h:7138: return IsFixedTypedArrayElementsKind(kind) ?
Representation::Integer32()
How about rewriting to:

if (IsFixedTypedArrayElementsKind(kind)) {
  return Representation::Integer32();
}

return Representation::Tagged();

[Jarin, 2015-07-27 12:28:34]

lgtm

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-properties.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-properties.cc:25: // elements kind to get coverage
for both access patterns:
The colon does not really make sense and it is not clear what 'both' means here.

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-properties.cc:88: // -
IsExternalArrayElementsKind(y)
Same here.

[jochen (gone - plz use gerrit), 2015-07-27 12:51:50]

https://codereview.chromium.org/1254623002/diff/60001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/compiler/access-bui...
src/compiler/access-builder.cc:84: FieldAccess
AccessBuilder::ForExternalArrayPointer() {
On 2015/07/27 at 12:28:01, Benedikt Meurer wrote:
> Nit: Please remove this method; it is unused anyway, and misleading.

done

https://codereview.chromium.org/1254623002/diff/60001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/factory.cc#newcode886
src/factory.cc:886: Handle<FixedTypedArrayBase> Factory::NewExternalArray(
On 2015/07/27 at 12:28:01, Benedikt Meurer wrote:
> Nit: Can we rename this method to NewFixedTypedArray (WithExternalPointer) ?

done

https://codereview.chromium.org/1254623002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3948: AllocationResult Heap::AllocateExternalArray(int length,
On 2015/07/27 at 12:28:01, Benedikt Meurer wrote:
> Nit: Same here, rename to AllocateFixedTypedArray ?

done

https://codereview.chromium.org/1254623002/diff/60001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1254623002/diff/60001/src/hydrogen-instructio...
src/hydrogen-instructions.h:7138: return IsFixedTypedArrayElementsKind(kind) ?
Representation::Integer32()
On 2015/07/27 at 12:28:01, Benedikt Meurer wrote:
> How about rewriting to:
> 
> if (IsFixedTypedArrayElementsKind(kind)) {
>   return Representation::Integer32();
> }
> 
> return Representation::Tagged();

done

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-properties.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-properties.cc:25: // elements kind to get coverage
for both access patterns:
On 2015/07/27 at 12:28:34, jarin wrote:
> The colon does not really make sense and it is not clear what 'both' means
here.

clarified

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-properties.cc:88: // -
IsExternalArrayElementsKind(y)
On 2015/07/27 at 12:28:34, jarin wrote:
> Same here.

done

[jochen (gone - plz use gerrit), 2015-07-27 12:51:56]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-07-27 12:51:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, bmeurer@chromium.org, hpayer@chromium.org,
jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/1254623002/#ps80001
(title: "updates")

[commit-bot: I haz the power, 2015-07-27 12:52:03]

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

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-27 13:19:47]

Patchset 5 (id:??) landed as
https://crrev.com/607ef7c6009a24ebf195b4cab7b0b436c5afd21c
Cr-Commit-Position: refs/heads/master@{#29872}

[Michael Achenbach, 2015-07-27 20:31:29]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1257223002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks several layout tests, e.g.:
http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2032/buil...

Several output lines change from PASS to FAIL. If the changes are intended,
please land a needsmanualrebaseline change in blink first..

[Michael Achenbach, 2015-07-28 07:31:37]

On 2015/07/27 20:31:29, Michael Achenbach wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/1257223002/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: [Sheriff] Breaks several layout tests, e.g.:
>
http://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2032/buil...
> 
> Several output lines change from PASS to FAIL. If the changes are intended,
> please land a needsmanualrebaseline change in blink first..

After the revert it got clear that this also caused the following:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20s...