[Interpreter] Add support for property load operations.

[Interpreter] Add support for property load operations.

Adds support for property load operations via Load/KeyedLoad ICs. Adds the
following bytecodes:
 - LoadIC
 - KeyedLoadIC
Also adds support to the interpreter assembler for loading the type feedback
vector from the function on the stack, and calling ICs.

BUG=v8:4280
LOG=N

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

[rmcilroy, 2015-09-01 13:15:28]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2015-09-01 13:17:46]

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

[commit-bot: I haz the power, 2015-09-01 13:17:54]

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

[rmcilroy, 2015-09-01 13:19:26]

rmcilroy@chromium.org changed reviewers:
+ mstarzinger@chromium.org, mvstanton@chromium.org, oth@chromium.org

[rmcilroy, 2015-09-01 13:19:27]

Adds support for loadICs to the interpreter.

Michi for TF stuff
Michael for TypeFeedbackVector stuff
Orion for bytecode-generator/array-builder stuff

PTAL, thanks!

[commit-bot: I haz the power, 2015-09-01 13:22:05]

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

[commit-bot: I haz the power, 2015-09-01 13:22:06]

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

[rmcilroy, 2015-09-01 13:55:11]

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

[commit-bot: I haz the power, 2015-09-01 13:55:17]

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

[oth, 2015-09-01 14:01:54]

Looks good here. One minor comment about template use.

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

https://codereview.chromium.org/1309843007/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:303: bool
BytecodeArrayBuilder::FitsByteOperand(T value) {
Template seems a bit gratuitous here - it's invoked with value being size_t or
int. For template, is_integral would clarify things.

FitsInByteOperand? ValueFitsInByteOperand?

[commit-bot: I haz the power, 2015-09-01 14:04:02]

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

[commit-bot: I haz the power, 2015-09-01 14:04:02]

Dry run: 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/9333)

[mvstanton, 2015-09-01 14:28:30]

Vector lgtm, a couple of comments - love the tests.

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

https://codereview.chromium.org/1309843007/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:342: ExpectedSnippet<const
char*> snippets[] = {
Nice test! Helps to explain what is happening...

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:239: Node*
InterpreterAssembler::CallIC(CallInterfaceDescriptor descriptor,
If the number of arguments are going to be fixed I'd like to see something in
the name calling it a LoadIC, since StoreIC and other types of ICs may have
different numbers of arguments.

Maybe an ic_kind (Code::LOAD_IC, etc.) parameter could be passed?

[rmcilroy, 2015-09-01 15:53:36]

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

https://codereview.chromium.org/1309843007/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:303: bool
BytecodeArrayBuilder::FitsByteOperand(T value) {
On 2015/09/01 14:01:54, oth wrote:
> Template seems a bit gratuitous here - it's invoked with value being size_t or
> int. For template, is_integral would clarify things.
> 
> FitsInByteOperand? ValueFitsInByteOperand?
> 

Ahh, is_integral is exactly what I wanted, thanks. Changed to FitsInByteOperand

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

https://codereview.chromium.org/1309843007/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:342: ExpectedSnippet<const
char*> snippets[] = {
On 2015/09/01 14:28:29, mvstanton wrote:
> Nice test! Helps to explain what is happening...

Acknowledged.

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:239: Node*
InterpreterAssembler::CallIC(CallInterfaceDescriptor descriptor,
On 2015/09/01 14:28:29, mvstanton wrote:
> If the number of arguments are going to be fixed I'd like to see something in
> the name calling it a LoadIC, since StoreIC and other types of ICs may have
> different numbers of arguments.
> 
> Maybe an ic_kind (Code::LOAD_IC, etc.) parameter could be passed?

My plan was to have overloads for the number of parameters once we need other
CallICs with a different number of arguments (similar to CallJSBuiltin below). I
didn't want to make these specific to a particular IC type, this is really just
meant to be a low-level operator which calls the given descriptor with the given
arguments. WDYT?

[mvstanton, 2015-09-01 16:54:13]

still lgtm

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1309843007/diff/40001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:239: Node*
InterpreterAssembler::CallIC(CallInterfaceDescriptor descriptor,
On 2015/09/01 15:53:36, rmcilroy wrote:
> On 2015/09/01 14:28:29, mvstanton wrote:
> > If the number of arguments are going to be fixed I'd like to see something
in
> > the name calling it a LoadIC, since StoreIC and other types of ICs may have
> > different numbers of arguments.
> > 
> > Maybe an ic_kind (Code::LOAD_IC, etc.) parameter could be passed?
> 
> My plan was to have overloads for the number of parameters once we need other
> CallICs with a different number of arguments (similar to CallJSBuiltin below).
I
> didn't want to make these specific to a particular IC type, this is really
just
> meant to be a low-level operator which calls the given descriptor with the
given
> arguments. WDYT?

Okay I can live with that :).

[Michael Starzinger, 2015-09-02 08:16:14]

Looking good, just one issue about evaluation order.

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:318: Visit(expr->obj());
The JavaScript evaluation order is first to evaluate the object and then
evaluate the key. That is observable with e.g. ...

(o.something_that_prints())[o.something_that_throws()];

[rmcilroy, 2015-09-02 11:21:20]

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1309843007/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:318: Visit(expr->obj());
On 2015/09/02 08:16:14, Michael Starzinger wrote:
> The JavaScript evaluation order is first to evaluate the object and then
> evaluate the key. That is observable with e.g. ...
> 
> (o.something_that_prints())[o.something_that_throws()];

Thanks, I should have paid more attention to this! Done, by changing round the
LoadIC bytecode to expect "name" in the accumulator and "object" in the named
register.

This is actually better overall, since once we can eliminate redundant register
load/store operations the common case (where the object is a local or a
parameter) can turn into:
  LoadConstant <name>
  LoadIC <local> <slot>

instead of the previous:
  LoadConstant <name>
  Star <tmp> 
  Lrda <local>
  LoadIC <tmp> <slot>

[Michael Starzinger, 2015-09-02 11:31:51]

LGTM from my end.

[rmcilroy, 2015-09-02 11:43:16]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-09-02 11:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/1309843007/#ps80001
(title: "Change evaluation order")

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

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

[commit-bot: I haz the power, 2015-09-02 11:46:31]

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

[commit-bot: I haz the power, 2015-09-02 11:46:31]

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

[rmcilroy, 2015-09-02 11:55:39]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-09-02 11:55:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1309843007/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-02 11:55:48]

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

[commit-bot: I haz the power, 2015-09-02 11:58:31]

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

[commit-bot: I haz the power, 2015-09-02 11:58:32]

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/9395)

[rmcilroy, 2015-09-02 12:37:54]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-09-02 12:37:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1309843007/#ps120001
(title: "Remove templated FitsInOperand since std::is_integral is not supported
on Mac")

[commit-bot: I haz the power, 2015-09-02 12:37:58]

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

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

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-09-02 13:03:29]

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