[Interpreter] Add support for loading literals from the constant pool.

[Interpreter] Add support for loading literals from the constant pool.

Adds support to the interpreter for loading literals from the constant pool.
Adds the LoadConstant bytecode and makes use of it for loading large Smis and
HeapObject literals.

Also removes unused HandleVector from utils.h.

BUG=v8:4280
LOG=N

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

[rmcilroy, 2015-08-27 15:31:44]

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

[commit-bot: I haz the power, 2015-08-27 15:31:56]

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

[commit-bot: I haz the power, 2015-08-27 15:35:45]

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

[commit-bot: I haz the power, 2015-08-27 15:35:45]

Dry run: Try jobs failed on following builders:
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[rmcilroy, 2015-08-27 15:52:18]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-08-27 15:52:22]

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

[commit-bot: I haz the power, 2015-08-27 15:52:34]

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

[commit-bot: I haz the power, 2015-08-27 15:57:48]

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

[commit-bot: I haz the power, 2015-08-27 15:57:49]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[rmcilroy, 2015-08-27 16:14:40]

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

[rmcilroy, 2015-08-27 16:14:47]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2015-08-27 16:14:53]

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

[rmcilroy, 2015-08-27 16:34:07]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[rmcilroy, 2015-08-27 16:34:07]

Michi, could you take a look please? Thanks.

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
Michi - looks like double literals aren't de-duped by the parser (we get a new
HeapNumber each time). Is this done elsewhere or have we always had a new
HeapNumber for each double literal?

[commit-bot: I haz the power, 2015-08-27 16:45:52]

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

[commit-bot: I haz the power, 2015-08-27 16:45:53]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-08-27 19:36:29]

Just replying to comment. Will do review tomorrow.

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
On 2015/08/27 16:34:07, rmcilroy wrote:
> Michi - looks like double literals aren't de-duped by the parser (we get a new
> HeapNumber each time). Is this done elsewhere or have we always had a new
> HeapNumber for each double literal?

Not sure I understand your comment correctly. How would the parser de-dupe
literals? That would make the AST degenerate into a DAG if we would do that. Or
am I missing something here?

Compilers can de-dupe the values. One mechanism of de-duping would be GVN which
makes sure that identical nodes in the graph are coalesced. Both Hydrogen and
TurboFan do that.

[Michael Starzinger, 2015-08-27 19:55:36]

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
On 2015/08/27 19:36:29, Michael Starzinger wrote:
> On 2015/08/27 16:34:07, rmcilroy wrote:
> > Michi - looks like double literals aren't de-duped by the parser (we get a
new
> > HeapNumber each time). Is this done elsewhere or have we always had a new
> > HeapNumber for each double literal?
> 
> Not sure I understand your comment correctly. How would the parser de-dupe
> literals? That would make the AST degenerate into a DAG if we would do that.
Or
> am I missing something here?
> 
> Compilers can de-dupe the values. One mechanism of de-duping would be GVN
which
> makes sure that identical nodes in the graph are coalesced. Both Hydrogen and
> TurboFan do that.

Ah, I think now I understand where you are coming from, I think your question
was whether AstValue::Internalize should canonicalize HeapNumbers or not. Maeh,
the data-structure for canonicalizing them would probably end up eating more
memory than the duped HeapNumbers, I don't think that would buy us anything
besides complexity.

Rather the question is, why do we "internalize" double literals in the first
place. One could argue that it should be the responsibility of the compiler to
decide whether and where to allocate HeapNumbers. But that's opening a can of
worms. I suggest we don't go there.

[titzer, 2015-08-27 20:01:29]

On 2015/08/27 19:55:36, Michael Starzinger wrote:
>
https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
> File test/cctest/interpreter/test-bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
> test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
> On 2015/08/27 19:36:29, Michael Starzinger wrote:
> > On 2015/08/27 16:34:07, rmcilroy wrote:
> > > Michi - looks like double literals aren't de-duped by the parser (we get a
> new
> > > HeapNumber each time). Is this done elsewhere or have we always had a new
> > > HeapNumber for each double literal?
> > 
> > Not sure I understand your comment correctly. How would the parser de-dupe
> > literals? That would make the AST degenerate into a DAG if we would do that.
> Or
> > am I missing something here?
> > 
> > Compilers can de-dupe the values. One mechanism of de-duping would be GVN
> which
> > makes sure that identical nodes in the graph are coalesced. Both Hydrogen
and
> > TurboFan do that.
> 
> Ah, I think now I understand where you are coming from, I think your question
> was whether AstValue::Internalize should canonicalize HeapNumbers or not.
Maeh,
> the data-structure for canonicalizing them would probably end up eating more
> memory than the duped HeapNumbers, I don't think that would buy us anything
> besides complexity.
> 
> Rather the question is, why do we "internalize" double literals in the first
> place. One could argue that it should be the responsibility of the compiler to
> decide whether and where to allocate HeapNumbers. But that's opening a can of
> worms. I suggest we don't go there.

The JSGraph in TurboFan de-dupes all constants (including HeapNumbers), except
other HeapObjects. I agree with Michi that the parser shouldn't try to de-dup
doubles; it's probably not worth it.

[rmcilroy, 2015-08-28 09:22:35]

On 2015/08/27 20:01:29, titzer wrote:
> On 2015/08/27 19:55:36, Michael Starzinger wrote:
> >
>
https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
> > File test/cctest/interpreter/test-bytecode-generator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1321663003/diff/40001/test/cctest/interpreter...
> > test/cctest/interpreter/test-bytecode-generator.cc:257: { 3.14, 3.14 }
> > On 2015/08/27 19:36:29, Michael Starzinger wrote:
> > > On 2015/08/27 16:34:07, rmcilroy wrote:
> > > > Michi - looks like double literals aren't de-duped by the parser (we get
a
> > new
> > > > HeapNumber each time). Is this done elsewhere or have we always had a
new
> > > > HeapNumber for each double literal?
> > > 
> > > Not sure I understand your comment correctly. How would the parser de-dupe
> > > literals? That would make the AST degenerate into a DAG if we would do
that.
> > Or
> > > am I missing something here?
> > > 
> > > Compilers can de-dupe the values. One mechanism of de-duping would be GVN
> > which
> > > makes sure that identical nodes in the graph are coalesced. Both Hydrogen
> and
> > > TurboFan do that.
> > 
> > Ah, I think now I understand where you are coming from, I think your
question
> > was whether AstValue::Internalize should canonicalize HeapNumbers or not.
> Maeh,
> > the data-structure for canonicalizing them would probably end up eating more
> > memory than the duped HeapNumbers, I don't think that would buy us anything
> > besides complexity.
> > 
> > Rather the question is, why do we "internalize" double literals in the first
> > place. One could argue that it should be the responsibility of the compiler
to
> > decide whether and where to allocate HeapNumbers. But that's opening a can
of
> > worms. I suggest we don't go there.
> 
> The JSGraph in TurboFan de-dupes all constants (including HeapNumbers), except
> other HeapObjects. I agree with Michi that the parser shouldn't try to de-dup
> doubles; it's probably not worth it.

Right, sorry I wasn't so clear here. I meant canonicalizing doubles, not GVN or
anything like that. I wasn't suggesting we change the parser to do this, I just
wanted to make sure I wasn't missing something (e.g., Strings are canonicalized,
just wanted to make sure I wasn't missing something similar for double
literals).

If we were going to this I agree it should be the responsibility of the
compliler / bytecode-array-builder (only canonicaling on a per-function level),
but it seems from your comments that this is unlikely to be much of a benefit,
so I'm quite happy to leave it as a comment for now and possibly investigate it
later once we can run real JS.

[Michael Starzinger, 2015-08-28 12:21:04]

Looking good. Just a few comments.

https://codereview.chromium.org/1321663003/diff/40001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1321663003/diff/40001/src/DEPS#newcode8
src/DEPS:8: "+src/heap/identity-map.h",
I have moved the IdentityMap out of the "heap" directory. This should no longer
be needed if you rebase.

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h
File src/heap/identity-map.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h...
src/heap/identity-map.h:9: // Do not include anything from src/heap here!
Likewise.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:55: if (constants_.size() == 0) {
This special-casing is already part of Factory::NewFixedArray, it should fall
out naturally of the code below.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:27: explicit
BytecodeArrayBuilder(Isolate* isolate, Zone* zone);
nit: No longer needs to be marked as explicit.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:79: std::vector<uint8_t> bytecodes_;
Same comment from below applies here.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:83: std::vector<Handle<Object>>
constants_;
Could we use a ZoneVector here, that would provide us with accounting for the
(zone) memory footprint of the interpreter later (i.e. similar to --turbo-stats)

[rmcilroy, 2015-08-28 14:41:47]

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

[commit-bot: I haz the power, 2015-08-28 14:41:58]

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

[rmcilroy, 2015-08-28 14:42:05]

Review comments addressed, PTAL, thanks.

https://codereview.chromium.org/1321663003/diff/40001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1321663003/diff/40001/src/DEPS#newcode8
src/DEPS:8: "+src/heap/identity-map.h",
Done (thanks for doing the move!)

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h
File src/heap/identity-map.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/heap/identity-map.h...
src/heap/identity-map.h:9: // Do not include anything from src/heap here!
On 2015/08/28 12:21:04, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:55: if (constants_.size() == 0) {
On 2015/08/28 12:21:04, Michael Starzinger wrote:
> This special-casing is already part of Factory::NewFixedArray, it should fall
> out naturally of the code below.

Perfect! thanks, done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:27: explicit
BytecodeArrayBuilder(Isolate* isolate, Zone* zone);
On 2015/08/28 12:21:04, Michael Starzinger wrote:
> nit: No longer needs to be marked as explicit.

Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:79: std::vector<uint8_t> bytecodes_;
On 2015/08/28 12:21:04, Michael Starzinger wrote:
> Same comment from below applies here.

Done.

https://codereview.chromium.org/1321663003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:83: std::vector<Handle<Object>>
constants_;
On 2015/08/28 12:21:04, Michael Starzinger wrote:
> Could we use a ZoneVector here, that would provide us with accounting for the
> (zone) memory footprint of the interpreter later (i.e. similar to
--turbo-stats)

Done.

[commit-bot: I haz the power, 2015-08-28 15:08:14]

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

[commit-bot: I haz the power, 2015-08-28 15:08:14]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-08-28 15:08:34]

LGTM.

[rmcilroy, 2015-08-28 15:39:01]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-08-28 15:39:09]

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

[commit-bot: I haz the power, 2015-08-28 15:41:00]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-08-28 15:41:15]

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