[Interpreter] Add support for Load / Store to Lookup slots.

[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.

BUG=v8:4280
LOG=N

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

[mythria, 2015-12-14 15:59:07]

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

[mythria, 2015-12-14 15:59:08]

Could you please review my changes.

Thanks,
Mythri

[Michael Starzinger, 2015-12-14 21:25:04]

https://codereview.chromium.org/1524803003/diff/1/src/runtime/runtime-interpr...
File src/runtime/runtime-interpreter.cc (right):

https://codereview.chromium.org/1524803003/diff/1/src/runtime/runtime-interpr...
src/runtime/runtime-interpreter.cc:232:
RUNTIME_FUNCTION(Runtime_InterpreterLoadLookupSlotNoReferenceErrorValue) {
The first projection should actually be usable without any special support.
IIRC, the result register for a runtime function with one result value coincides
with the first result of a runtime with a pair as a result value. So I think
this wrapper is not necessary.

https://codereview.chromium.org/1524803003/diff/1/src/runtime/runtime-interpr...
src/runtime/runtime-interpreter.cc:235: return result.x;
This won't compile on 32-bit architectures.

[Michael Starzinger, 2015-12-14 21:50:55]

Also please don't base this CL on Ross' CL adding support to calling eval. I
think if we rebase this on tip-of-tree and use the existing runtime functions,
then this should all just work. 

Here is my theory, please let me know if I am off somewhere:  All we will need
is a slight adaption of InterpreterAssembler::CallRuntime to use the correct
CEntryStub based on Runtime::FunctionForId(id)->result_size (that's only needed
for PPC and WIN64 in the first place). Then we can just use the existing
Runtime_LoadLookupSlot and Runtime_LoadLookupSlotNoReferenceError unaltered and
directly.

[Michael Starzinger, 2015-12-14 22:03:20]

Description was changed from

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.

Temporarily introduces Runtime::kInterpreterLookupSlotNoReferenceErrorValue
to the runtime. This can be removed when interpreter assembler supports
projection on Nodes.

BUG=V8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.

Temporarily introduces Runtime::kInterpreterLookupSlotNoReferenceErrorValue
to the runtime. This can be removed when interpreter assembler supports
projection on Nodes.

BUG=v8:4280
LOG=N
==========

[mythria, 2015-12-15 11:34:16]

I removed the interpreter runtime functions to handle multiple return values. I
use projection instead. If I directly use the value without projecting it,
verifier fails. It verifies that all successor operations of a node returning
multiple values is a projection operation. I also felt it is safer to have a
projection operation rather than relying on the where the first result of the
result pair is stored.

I did not change CallRuntime function to handle multiple results. For Load/Store
Lookup slots we use a version that calls CallRuntime of RawMachineAssembler
which already handles multiple results. For StoreLookupSlot we use CallRuntime4,
and for LoadLookupSlot we use CallRuntime2 of RawMachineAssembler. 

I did not rebase it yet. I will rebase it and fix the tests in another patch. 

Please take a look and let me know if these changes are ok. 

Thanks,
Mythri

[mythria, 2015-12-15 11:42:30]

Description was changed from

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.

Temporarily introduces Runtime::kInterpreterLookupSlotNoReferenceErrorValue
to the runtime. This can be removed when interpreter assembler supports
projection on Nodes.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.


BUG=v8:4280
LOG=N
==========

[Michael Starzinger, 2015-12-15 11:52:16]

As discussed offline: Yes, approach looks good now. Thanks!

[mythria, 2015-12-15 15:25:40]

I rebased the patch, so there is no dependency on Ross's eval cl now. I also
fixed tests because we have no support for eval now. I have very few tests. I
will add more when we have eval. Please take a look.

Thanks,
Mythri

[Michael Starzinger, 2015-12-15 16:01:49]

LGTM.

[rmcilroy, 2015-12-15 16:44:33]

LGTM with comments addressed.

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

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:446: size_t name_index =
GetConstantPoolEntry(name);
Nice, as a separate CL, could you update the LoadNamedProperty and other
bytecodes which take a name_index to instead take Handle<Name> and get the
constant pool entry in the BytecodeArrayBuilder like this.

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:447: if
(FitsInIdx8Operand(name_index)) {
You could create a wide version while you're at it (in another CL is fine).

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:69: /* Load-Store LookupSlots */                    
                            \
/s/LookupSlots/lookup slots/

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:453: __ SetAccumulator(result);
Please pull out common code to a helper function, passing the runtime function
id (Runtime::kLoadLookupSlot or  Runtime::kLoadLookupSlotNoReferenceError) as an
argument.

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:486: Node* language_mode = __
NumberConstant(LanguageMode::STRICT);
Ditto (passing language mode).

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

https://codereview.chromium.org/1524803003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:70: return
handle(js_function->shared()->bytecode_array(), CcTest::i_isolate());
nit - pull out common code with MakeTopLevelBytecode (and potentially
MakeBytecode(script, function_name))

https://codereview.chromium.org/1524803003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:5544: "  eval(\"function t()
{";
nit - use ' instead of \"

[mythria, 2015-12-16 09:54:21]

Thanks for your reviews. I addressed review comments.

Thanks,
Mythri

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

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:447: if
(FitsInIdx8Operand(name_index)) {
On 2015/12/15 16:44:32, rmcilroy wrote:
> You could create a wide version while you're at it (in another CL is fine).

I will do it in another cl. with more tests.

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:69: /* Load-Store LookupSlots */                    
                            \
On 2015/12/15 16:44:32, rmcilroy wrote:
> /s/LookupSlots/lookup slots/

Done.

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:453: __ SetAccumulator(result);
On 2015/12/15 16:44:33, rmcilroy wrote:
> Please pull out common code to a helper function, passing the runtime function
> id (Runtime::kLoadLookupSlot or  Runtime::kLoadLookupSlotNoReferenceError) as
an
> argument.

Done.

https://codereview.chromium.org/1524803003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:486: Node* language_mode = __
NumberConstant(LanguageMode::STRICT);
On 2015/12/15 16:44:32, rmcilroy wrote:
> Ditto (passing language mode).

Done.

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

https://codereview.chromium.org/1524803003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:70: return
handle(js_function->shared()->bytecode_array(), CcTest::i_isolate());
On 2015/12/15 16:44:33, rmcilroy wrote:
> nit - pull out common code with MakeTopLevelBytecode (and potentially
> MakeBytecode(script, function_name))

Done.

https://codereview.chromium.org/1524803003/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:5544: "  eval(\"function t()
{";
On 2015/12/15 16:44:33, rmcilroy wrote:
> nit - use ' instead of \"

I use 'use strict'; in function body in one example.So I used \".

[rmcilroy, 2015-12-16 12:07:25]

LGTM with two final nits, thanks!

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.h:99: void LdaLookupSlotHelper(Runtime::FunctionId
function_id,
Just call this DoLoadLookupSlot to be consistent with the other helpers above.

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.h:103: void StaLookupSlotHelper(LanguageMode
language_mode,
DoStoreLookupSlot for the same reason.

[mythria, 2015-12-16 16:44:01]

Thanks

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.h:99: void LdaLookupSlotHelper(Runtime::FunctionId
function_id,
On 2015/12/16 12:07:25, rmcilroy wrote:
> Just call this DoLoadLookupSlot to be consistent with the other helpers above.

Done.

https://codereview.chromium.org/1524803003/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.h:103: void StaLookupSlotHelper(LanguageMode
language_mode,
On 2015/12/16 12:07:25, rmcilroy wrote:
> DoStoreLookupSlot for the same reason.

Done.

[mythria, 2015-12-16 17:22:52]

The CQ bit was checked by mythria@chromium.org

[mythria, 2015-12-16 17:22:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1524803003/#ps80001
(title: "Fixed nits.")

[commit-bot: I haz the power, 2015-12-16 17:23:04]

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

[commit-bot: I haz the power, 2015-12-16 17:24:23]

Description was changed from

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.


BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.


BUG=v8:4280
LOG=N
==========

[commit-bot: I haz the power, 2015-12-16 17:24:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-16 17:25:07]

Description was changed from

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.


BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add support for Load / Store to Lookup slots.

Adds support for loading and storing lookup variables.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/a4162898d8d3570e656f981ad82a47035668bb07
Cr-Commit-Position: refs/heads/master@{#32913}
==========

[commit-bot: I haz the power, 2015-12-16 17:25:07]

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