Introduce LiteralsArray to hide it's implementation.

Introduce LiteralsArray to hide it's implementation.

The LiteralsArray will soon hold a type feedback vector. Code treats it as an
ordinary fixed array, and needs to stop that.

BUG=

Committed: https://crrev.com/d8cdd6956a04cfcf03159ef7802c44838eee3146
Cr-Commit-Position: refs/heads/master@{#31000}

[mvstanton, 2015-09-28 14:48:32]

mvstanton@chromium.org changed reviewers:
+ ishell@chromium.org, jkummerow@chromium.org

[mvstanton, 2015-09-28 14:48:32]

Hi Igor,
Here is the patch we discussed. The LiteralsArray type puts the existing vector
from the SharedFunctionInfo in index 0, and the literals are displaced by 1. In
future, we'll create the vector at that location.

I like to "waste" that word for now because it flushes out anybody using the
literals array incorrectly.

Jakob, could you look at full codegen?
I'm working on the ports and will write again when ready...
--Michael

[mvstanton, 2015-09-28 14:54:29]

Okay, ports komplett.

[Igor Sheludko, 2015-09-28 15:16:46]

nits:

https://codereview.chromium.org/1374723002/diff/40001/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:5401: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
Shouldn't " + LiteralsArray::kFirstLiteralIndex" be done inside the method?
WDYT about introducing LiteralsArray::OffsetOfLiteralAt()?

https://codereview.chromium.org/1374723002/diff/40001/src/arm64/lithium-codeg...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/arm64/lithium-codeg...
src/arm64/lithium-codegen-arm64.cc:5651: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1501: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
LiteralsArray::OffsetOfLiteralAt()?

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm64/...
File src/full-codegen/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm64/...
src/full-codegen/arm64/full-codegen-arm64.cc:1490: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/ia32/f...
File src/full-codegen/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/ia32/f...
src/full-codegen/ia32/full-codegen-ia32.cc:1425: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/mips/f...
File src/full-codegen/mips/full-codegen-mips.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/mips/f...
src/full-codegen/mips/full-codegen-mips.cc:1500: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/mips64...
File src/full-codegen/mips64/full-codegen-mips64.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/mips64...
src/full-codegen/mips64/full-codegen-mips64.cc:1498: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/x64/fu...
File src/full-codegen/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/x64/fu...
src/full-codegen/x64/full-codegen-x64.cc:1456: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/ia32/lithium-codege...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/ia32/lithium-codege...
src/ia32/lithium-codegen-ia32.cc:5284: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
Shouldn't " + LiteralsArray::kFirstLiteralIndex" be done inside the method?

https://codereview.chromium.org/1374723002/diff/40001/src/mips/lithium-codege...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/mips/lithium-codege...
src/mips/lithium-codegen-mips.cc:5417: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
Same here.

https://codereview.chromium.org/1374723002/diff/40001/src/mips64/lithium-code...
File src/mips64/lithium-codegen-mips64.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/mips64/lithium-code...
src/mips64/lithium-codegen-mips64.cc:5602: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
Same here.

[mvstanton, 2015-09-28 16:01:33]

Thanks Igor. All comments addressed.

https://codereview.chromium.org/1374723002/diff/40001/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:5401: instr->hydrogen()->literal_index() +
LiteralsArray::kFirstLiteralIndex);
On 2015/09/28 15:16:45, Igor Sheludko wrote:
> Shouldn't " + LiteralsArray::kFirstLiteralIndex" be done inside the method?
> WDYT about introducing LiteralsArray::OffsetOfLiteralAt()?

I'd like all places that talk about a literal index mean the literal index as
assigned as AST time. These are 0, 1, .... 

It's only at the last moment in stubs or direct looks at the array that the
index is resolved to a FixedArray index.

I'll use OffsetOfLiteralAt here, good idea.

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1374723002/diff/40001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1501: int literal_offset =
LiteralsArray::kOffsetToFirstLiteral +
On 2015/09/28 15:16:45, Igor Sheludko wrote:
> LiteralsArray::OffsetOfLiteralAt()?

Done.

[Igor Sheludko, 2015-09-28 16:35:28]

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

https://codereview.chromium.org/1374723002/diff/60001/src/factory.cc#newcode2094
src/factory.cc:2094: NewFixedArray(LiteralsArray::SizeFor(number_of_literals),
pretenure);
I'm afraid that here we will actually call FixedArray::SizeFor() which doesn't
know about additional slots. Maybe we should add LiteralsArray:SizeFor() and
implement OffsetOfLiteralAt() in terms of SizeFor().

https://codereview.chromium.org/1374723002/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1374723002/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:3501: int LiteralsArray::literals_count() const { return
length() - 1; }
- kFirstLiteralIndex ?

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h#newcode4563
src/objects.h:4563: static int OffsetOfLiteralAt(int index) { return
SizeFor(index + 1); }
+ kFirstLiteralIndex ?

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h#newcode4571
src/objects.h:4571: DECLARE_CAST(LiteralsArray)
Can we add FixedArray's get/set methods to private part of LiteralsArray in
order to enforce literal/set_literal usage?

[mvstanton, 2015-09-29 08:25:13]

Hi Igor,
Comments addressed. Thanks!
--Michael

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

https://codereview.chromium.org/1374723002/diff/60001/src/factory.cc#newcode2094
src/factory.cc:2094: NewFixedArray(LiteralsArray::SizeFor(number_of_literals),
pretenure);
On 2015/09/28 16:35:27, Igor Sheludko wrote:
> I'm afraid that here we will actually call FixedArray::SizeFor() which doesn't
> know about additional slots. Maybe we should add LiteralsArray:SizeFor() and
> implement OffsetOfLiteralAt() in terms of SizeFor().

Okay, I see I was using SizeFor incorrectly. It is a method that returns the
number of bytes needed for the whole FixedArray, whereas creating the array here
we need the number of elements. So how about if I move the actual creation
inside of LiteralsArray, and the factory method is just here as the canonical
way to access it. Because, why should factory.cc know that a LiteralsArray is a
fixed array of size length+1?

https://codereview.chromium.org/1374723002/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1374723002/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:3501: int LiteralsArray::literals_count() const { return
length() - 1; }
On 2015/09/28 16:35:27, Igor Sheludko wrote:
> - kFirstLiteralIndex ?

Done.

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h#newcode4563
src/objects.h:4563: static int OffsetOfLiteralAt(int index) { return
SizeFor(index + 1); }
On 2015/09/28 16:35:28, Igor Sheludko wrote:
> + kFirstLiteralIndex ?

Done.

https://codereview.chromium.org/1374723002/diff/60001/src/objects.h#newcode4571
src/objects.h:4571: DECLARE_CAST(LiteralsArray)
On 2015/09/28 16:35:28, Igor Sheludko wrote:
> Can we add FixedArray's get/set methods to private part of LiteralsArray in
> order to enforce literal/set_literal usage?

Good idea, done.

[Igor Sheludko, 2015-09-29 08:40:42]

lgtm with nits:

https://codereview.chromium.org/1374723002/diff/80001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1374723002/diff/80001/src/factory.cc#newcode2090
src/factory.cc:2090: Handle<LiteralsArray> Factory::NewLiteralsArray(
I think we don't need this method, just call LiteralsArray::New() instead.

https://codereview.chromium.org/1374723002/diff/80001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1374723002/diff/80001/src/objects-inl.h#newco...
src/objects-inl.h:3475: Object* LiteralsArray::get(int index) const { return
FixedArray::get(index); }
I think you can even leave these methods unimplemented...

https://codereview.chromium.org/1374723002/diff/80001/src/objects-inl.h#newco...
src/objects-inl.h:3500: return TypeFeedbackVector::cast(get(kVectorIndex));
... if you use FixedArray::get/FixedArray::set methods instead of get/set.

https://codereview.chromium.org/1374723002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1374723002/diff/80001/src/objects.h#newcode4573
src/objects.h:4573: static Handle<LiteralsArray> Allocate(Isolate* isolate,
Maybe New() is a better name.

[mvstanton, 2015-09-29 09:43:38]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-09-29 09:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1374723002/#ps140001
(title: "REBASE.")

[commit-bot: I haz the power, 2015-09-29 09:43:57]

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

[commit-bot: I haz the power, 2015-09-29 09:47:37]

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

[commit-bot: I haz the power, 2015-09-29 09:47:38]

Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/8488)

[mvstanton, 2015-09-29 09:52:58]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-09-29 09:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1374723002/#ps150026
(title: "Fix build break.")

[commit-bot: I haz the power, 2015-09-29 09:53:09]

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

[commit-bot: I haz the power, 2015-09-29 10:15:37]

Committed patchset #9 (id:150026)

[commit-bot: I haz the power, 2015-09-29 10:15:48]

Patchset 9 (id:??) landed as
https://crrev.com/d8cdd6956a04cfcf03159ef7802c44838eee3146
Cr-Commit-Position: refs/heads/master@{#31000}