[Interpreter] Add function literal support.

[Interpreter] Add function literal support.

Adds function literal support and add support for OTHER_CALLS which can be
made when calling a function literal.

Adds the CreateClosure bytecode.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/66e593733742cd929df02597573d92a4f7bca353
Cr-Commit-Position: refs/heads/master@{#31231}

[rmcilroy, 2015-10-08 14:34:05]

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

[commit-bot: I haz the power, 2015-10-08 14:34:17]

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

[rmcilroy, 2015-10-08 14:35:11]

rmcilroy@chromium.org changed reviewers:
+ cbruni@chromium.org, oth@chromium.org

[rmcilroy, 2015-10-08 14:35:12]

Orion, Camillo, could you please take a look, thanks.

[commit-bot: I haz the power, 2015-10-08 14:58:07]

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

[commit-bot: I haz the power, 2015-10-08 14:58:07]

Dry run: This issue passed the CQ dry run.

[oth, 2015-10-09 10:54:21]

lgtm.

Reservations discussed offline as to whether there should be a separate bytecode
or not. Can live with it either way.

[rmcilroy, 2015-10-09 12:37:22]

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

[rmcilroy, 2015-10-09 12:37:22]

> Reservations discussed offline as to whether there should be a separate
bytecode
> or not. Can live with it either way.

Yeah, this probably makes sense. Updated to move to a CreateClosure bytecode.
PTAL.

Adding Michi for compiler/ changes.

[rmcilroy, 2015-10-09 12:41:57]

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

[commit-bot: I haz the power, 2015-10-09 12:42:09]

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

[commit-bot: I haz the power, 2015-10-09 13:04:07]

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

[commit-bot: I haz the power, 2015-10-09 13:04:08]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-10-09 13:19:00]

LGTM. Just nits and random ramblings.

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

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:451: CHECK(!shared_info.is_null());  //
TODO(rmcilroy): Set stack overflow?
Yeah, that same check in the AstGraphBuilder bothers me every time I see it. I
finally need to dig into whether this can ever happen, and if so, write a repro
for that. Let me know if it ever happens to hit you, I'd be interested. :)

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:743:
builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
nit: I don't think it matters much, since we don't have any deoptimization yet,
but could we store the receiver before we evaluate the {callee_expr}? Or is
there a particular reason for this order? It shouldn't matter but I would feel
more comfortable setting the receiver register first.

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

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:660: // TODO(rmcilroy): Possibly call
FastNewClosureStub when possible instead of
About this TODO: Michael Stanton is currently working on extending the
FestNewClosureStub. If we are lucky, that stub can soon be used without
restrictions on the literals array. So just give this TODO some resting time, it
might soon be much easier to address. :)

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

https://codereview.chromium.org/1396693003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:124: static void
CheckConstant(InstanceType expected, Object* actual) {
nit: How would you feel about s/CheckConstant/CheckInstanceType/ here?

[Michael Starzinger, 2015-10-09 13:28:01]

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

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:743:
builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
On 2015/10/09 13:19:00, Michael Starzinger wrote:
> nit: I don't think it matters much, since we don't have any deoptimization
yet,
> but could we store the receiver before we evaluate the {callee_expr}? Or is
> there a particular reason for this order? It shouldn't matter but I would feel
> more comfortable setting the receiver register first.

Actually, I just checked again and we already in full-codegen don't have the
receiver on the operand stack here. So my comment is completely without basis.
Please disregard, sorry for the noise.

[oth, 2015-10-11 21:27:13]

On 2015/10/09 12:37:22, rmcilroy wrote:
> > Reservations discussed offline as to whether there should be a separate
> bytecode
> > or not. Can live with it either way.
> 
> Yeah, this probably makes sense. Updated to move to a CreateClosure bytecode.
> PTAL.
> 
> Adding Michi for compiler/ changes.

Great, looks good here.
Thanks.

[Camillo Bruni, 2015-10-12 07:43:50]

LGTM

Not much to contribute yet, but I'm happy to be in the loop :)

[rmcilroy, 2015-10-12 17:00:15]

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

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:451: CHECK(!shared_info.is_null());  //
TODO(rmcilroy): Set stack overflow?
On 2015/10/09 13:19:00, Michael Starzinger wrote:
> Yeah, that same check in the AstGraphBuilder bothers me every time I see it. I
> finally need to dig into whether this can ever happen, and if so, write a
repro
> for that. Let me know if it ever happens to hit you, I'd be interested. :)

Yeah I've never seen this myself. I'll let you know if I ever hit it.

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:743:
builder()->LoadUndefined().StoreAccumulatorInRegister(receiver);
On 2015/10/09 13:28:01, Michael Starzinger wrote:
> On 2015/10/09 13:19:00, Michael Starzinger wrote:
> > nit: I don't think it matters much, since we don't have any deoptimization
> yet,
> > but could we store the receiver before we evaluate the {callee_expr}? Or is
> > there a particular reason for this order? It shouldn't matter but I would
feel
> > more comfortable setting the receiver register first.
> 
> Actually, I just checked again and we already in full-codegen don't have the
> receiver on the operand stack here. So my comment is completely without basis.
> Please disregard, sorry for the noise.

It doesn't actually matter which way round this goes, I was just following the
evaluation order of ASTGraphBuilder which does it. Swapped it around in any
case.

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

https://codereview.chromium.org/1396693003/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:660: // TODO(rmcilroy): Possibly call
FastNewClosureStub when possible instead of
On 2015/10/09 13:19:00, Michael Starzinger wrote:
> About this TODO: Michael Stanton is currently working on extending the
> FestNewClosureStub. If we are lucky, that stub can soon be used without
> restrictions on the literals array. So just give this TODO some resting time,
it
> might soon be much easier to address. :)

Good to know, thanks!

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

https://codereview.chromium.org/1396693003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:124: static void
CheckConstant(InstanceType expected, Object* actual) {
On 2015/10/09 13:19:00, Michael Starzinger wrote:
> nit: How would you feel about s/CheckConstant/CheckInstanceType/ here?

This would require rewriting the CheckBytecodeArrayEqual function since it
currently uses the template to work out which CheckConstant operator to call
based on the type of the constants used in the ExpectedSnippet. This was a way
of checking that the constants were the expected types since we can't check
their values for these tests. Would you be OK landing it as-is?

[rmcilroy, 2015-10-12 17:01:36]

Patchset #3 (id:40001) has been deleted

[Michael Starzinger, 2015-10-13 07:56:14]

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

https://codereview.chromium.org/1396693003/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:124: static void
CheckConstant(InstanceType expected, Object* actual) {
On 2015/10/12 17:00:15, rmcilroy wrote:
> On 2015/10/09 13:19:00, Michael Starzinger wrote:
> > nit: How would you feel about s/CheckConstant/CheckInstanceType/ here?
> 
> This would require rewriting the CheckBytecodeArrayEqual function since it
> currently uses the template to work out which CheckConstant operator to call
> based on the type of the constants used in the ExpectedSnippet. This was a way
> of checking that the constants were the expected types since we can't check
> their values for these tests. Would you be OK landing it as-is?  

Acknowledged. Yep, fine with me.

[rmcilroy, 2015-10-13 09:12:14]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-13 09:12:14]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
cbruni@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1396693003/#ps60001
(title: "Review comments")

[commit-bot: I haz the power, 2015-10-13 09:12:30]

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

[commit-bot: I haz the power, 2015-10-13 09:39:58]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-10-13 09:40:15]

Patchset 3 (id:??) landed as
https://crrev.com/66e593733742cd929df02597573d92a4f7bca353
Cr-Commit-Position: refs/heads/master@{#31231}