First simple implementation of class literals in TurboFan.

First simple implementation of class literals in TurboFan.

R=rossberg@chromium.org,jarin@chromium.org
TEST=cctest/test-run-jsops/ClassLiteral

Committed: https://crrev.com/0f418385d066c724b264e3f500f46e82b60acd61
Cr-Commit-Position: refs/heads/master@{#26101}

[Michael Starzinger, 2015-01-14 15:16:35]

Jarin: Please take a look at the new bailout ids.
Andreas: PTAL at the rest.

[arv (Not doing code reviews), 2015-01-14 15:49:09]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-01-14 15:49:10]

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:343: return
environment()->Push(jsgraph()->TheHoleConstant());
I could also have the parser use TheHole instead of NULL when there is no
extends clause.

https://codereview.chromium.org/798873006/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/types.cc#newcode254
src/types.cc:254: case SCRIPT_TYPE:
Stray change?

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (left):

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:196: # Skip endain dependent test for mips due to
different typed views of the same
Stray change?

[Michael Starzinger, 2015-01-14 16:02:20]

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:343: return
environment()->Push(jsgraph()->TheHoleConstant());
On 2015/01/14 15:49:10, arv wrote:
> I could also have the parser use TheHole instead of NULL when there is no
> extends clause.

Hmm, I don't have an opinion either way. On the one hand it would avoid having
this method. On the other hand we get canonicalization of the HeapConstant node
for free this way (i.e. JSGraph::HeapConstant is not yet canonicalized).
Whatever you think is cleaner.

https://codereview.chromium.org/798873006/diff/1/src/types.cc
File src/types.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/types.cc#newcode254
src/types.cc:254: case SCRIPT_TYPE:
On 2015/01/14 15:49:10, arv wrote:
> Stray change?

Nope, this is now needed because we place a HeapConstant node referencing a
v8::internal::Script object into the graph for the first time.

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (left):

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:196: # Skip endain dependent test for mips due to
different typed views of the same
On 2015/01/14 15:49:10, arv wrote:
> Stray change?

Yes. I wanted to sneak in this cleanup, because the PASS expectation is the
default anyways and the comment about this being specific to MIPS is bogus as
well. I can move this into a separate CL.

[Michael Starzinger, 2015-01-15 09:15:28]

Patchset #2 (id:20001) has been deleted

[Michael Starzinger, 2015-01-15 09:31:11]

I uploaded a small simplification of the block context creation in patch set #2.

[Jarin, 2015-01-15 10:43:59]

On 2015/01/15 09:31:11, Michael Starzinger wrote:
> I uploaded a small simplification of the block context creation in patch set
#2.

I am confused about the BailoutIds: I see you added them to the full code
generator, but I do not see how they are used on the turbofan side. Could you
point me there?

Moreover, are you sure that none of the new nodes in
AstGraphBuilder::VisitClassLiteralContents can deopt? It appears that bunch of
them do not have frame state attached. Have you tried to run the test with
--turbo-deoptimization?

[Michael Starzinger, 2015-01-15 10:49:59]

On 2015/01/15 10:43:59, jarin wrote:
> On 2015/01/15 09:31:11, Michael Starzinger wrote:
> > I uploaded a small simplification of the block context creation in patch set
> #2.
> 
> I am confused about the BailoutIds: I see you added them to the full code
> generator, but I do not see how they are used on the turbofan side. Could you
> point me there?
> 
> Moreover, are you sure that none of the new nodes in
> AstGraphBuilder::VisitClassLiteralContents can deopt? It appears that bunch of
> them do not have frame state attached. Have you tried to run the test with
> --turbo-deoptimization?

Yes, I added the BailoutIds and the bailout points are prepared in full codegen,
but they are never used in TurboFan, that is the state of this CL. Nope, I
haven't tried running with --turbo-deoptimization. Will do so.

[Michael Starzinger, 2015-01-15 12:48:45]

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (left):

https://codereview.chromium.org/798873006/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:196: # Skip endain dependent test for mips due to
different typed views of the same
On 2015/01/14 16:02:19, Michael Starzinger wrote:
> On 2015/01/14 15:49:10, arv wrote:
> > Stray change?
> 
> Yes. I wanted to sneak in this cleanup, because the PASS expectation is the
> default anyways and the comment about this being specific to MIPS is bogus as
> well. I can move this into a separate CL.

Done. Moved into separate CL and already landed.

[Michael Starzinger, 2015-01-15 15:12:35]

On 2015/01/15 10:49:59, Michael Starzinger wrote:
> On 2015/01/15 10:43:59, jarin wrote:
> > On 2015/01/15 09:31:11, Michael Starzinger wrote:
> > > I uploaded a small simplification of the block context creation in patch
set
> > #2.
> > 
> > I am confused about the BailoutIds: I see you added them to the full code
> > generator, but I do not see how they are used on the turbofan side. Could
you
> > point me there?
> > 
> > Moreover, are you sure that none of the new nodes in
> > AstGraphBuilder::VisitClassLiteralContents can deopt? It appears that bunch
of
> > them do not have frame state attached. Have you tried to run the test with
> > --turbo-deoptimization?
> 
> Yes, I added the BailoutIds and the bailout points are prepared in full
codegen,
> but they are never used in TurboFan, that is the state of this CL. Nope, I
> haven't tried running with --turbo-deoptimization. Will do so.

I can do you one better than running with --turbo-deoptimization, I can make
sure everyone runs with --turbo-deoptimization all the time. :)

See https://codereview.chromium.org/845193004/ for details.

[rossberg, 2015-01-15 15:31:19]

LGTM with nits

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:343: return
environment()->Push(jsgraph()->TheHoleConstant());
On 2015/01/14 16:02:19, Michael Starzinger wrote:
> On 2015/01/14 15:49:10, arv wrote:
> > I could also have the parser use TheHole instead of NULL when there is no
> > extends clause.
> 
> Hmm, I don't have an opinion either way. On the one hand it would avoid having
> this method. On the other hand we get canonicalization of the HeapConstant
node
> for free this way (i.e. JSGraph::HeapConstant is not yet canonicalized).
> Whatever you think is cleaner.

Using the hole here would be confusing levels. We consistently use NULL to
represent optional parts in the AST. The hole is a detail of code generation,
nothing to do with the AST.

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
It seems to me that none but MATERIALIZED_LITERAL should possibly appear here,
so we might want to mark the others unreachable (similarly, in full-codegens).

https://codereview.chromium.org/798873006/diff/60001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-jsops.cc (right):

https://codereview.chromium.org/798873006/diff/60001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-jsops.cc:534: "    x() { return a; }"
Maybe throw in a constructor, too.

[arv (Not doing code reviews), 2015-01-15 16:12:24]

LGTM

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
On 2015/01/15 15:31:19, rossberg wrote:
> It seems to me that none but MATERIALIZED_LITERAL should possibly appear here,
> so we might want to mark the others unreachable (similarly, in full-codegens).

I believe PROTOTYPE can happen too

class C {
  __proto__() {}
}

[rossberg, 2015-01-15 16:35:11]

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
On 2015/01/15 16:12:24, arv wrote:
> On 2015/01/15 15:31:19, rossberg wrote:
> > It seems to me that none but MATERIALIZED_LITERAL should possibly appear
here,
> > so we might want to mark the others unreachable (similarly, in
full-codegens).
> 
> I believe PROTOTYPE can happen too
> 
> class C {
>   __proto__() {}
> }

That is just considered a regular method definition, isn't it? As such, it
shouldn't have PROTOTYPE kind AFAICT.

[arv (Not doing code reviews), 2015-01-15 16:51:02]

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
On 2015/01/15 16:35:11, rossberg wrote:
> On 2015/01/15 16:12:24, arv wrote:
> > On 2015/01/15 15:31:19, rossberg wrote:
> > > It seems to me that none but MATERIALIZED_LITERAL should possibly appear
> here,
> > > so we might want to mark the others unreachable (similarly, in
> full-codegens).
> > 
> > I believe PROTOTYPE can happen too
> > 
> > class C {
> >   __proto__() {}
> > }
> 
> That is just considered a regular method definition, isn't it? As such, it
> shouldn't have PROTOTYPE kind AFAICT.

The AST only checks the value of the property name and since that matches
__proto__ it uses PROTOTYPE as the kind.

Let me refactor that in a different CL.

[Michael Starzinger, 2015-01-16 09:43:55]

Comments addressed.

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.cc:343: return
environment()->Push(jsgraph()->TheHoleConstant());
On 2015/01/15 15:31:19, rossberg wrote:
> On 2015/01/14 16:02:19, Michael Starzinger wrote:
> > On 2015/01/14 15:49:10, arv wrote:
> > > I could also have the parser use TheHole instead of NULL when there is no
> > > extends clause.
> > 
> > Hmm, I don't have an opinion either way. On the one hand it would avoid
having
> > this method. On the other hand we get canonicalization of the HeapConstant
> node
> > for free this way (i.e. JSGraph::HeapConstant is not yet canonicalized).
> > Whatever you think is cleaner.
> 
> Using the hole here would be confusing levels. We consistently use NULL to
> represent optional parts in the AST. The hole is a detail of code generation,
> nothing to do with the AST.

Acknowledged.

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
On 2015/01/15 16:51:02, arv wrote:
> On 2015/01/15 16:35:11, rossberg wrote:
> > On 2015/01/15 16:12:24, arv wrote:
> > > On 2015/01/15 15:31:19, rossberg wrote:
> > > > It seems to me that none but MATERIALIZED_LITERAL should possibly appear
> > here,
> > > > so we might want to mark the others unreachable (similarly, in
> > full-codegens).
> > > 
> > > I believe PROTOTYPE can happen too
> > > 
> > > class C {
> > >   __proto__() {}
> > > }
> > 
> > That is just considered a regular method definition, isn't it? As such, it
> > shouldn't have PROTOTYPE kind AFAICT.
> 
> The AST only checks the value of the property name and since that matches
> __proto__ it uses PROTOTYPE as the kind.
> 
> Let me refactor that in a different CL.

Done. Made CONSTANT and MATERIALIZED_LITERAL unreachable. Normally all
FunctionLiterals should be COMPUTED, still need PROTOTYPE as per Arv's
explanation. I left the full-codegens untouched.

https://codereview.chromium.org/798873006/diff/60001/test/cctest/compiler/tes...
File test/cctest/compiler/test-run-jsops.cc (right):

https://codereview.chromium.org/798873006/diff/60001/test/cctest/compiler/tes...
test/cctest/compiler/test-run-jsops.cc:534: "    x() { return a; }"
On 2015/01/15 15:31:19, rossberg wrote:
> Maybe throw in a constructor, too.

Done.

[rossberg, 2015-01-16 10:43:30]

Still lgtm

[Michael Starzinger, 2015-01-16 12:10:16]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2015-01-16 12:11:05]

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

[commit-bot: I haz the power, 2015-01-16 12:38:20]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-01-16 12:38:35]

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

[arv (Not doing code reviews), 2015-01-16 14:42:07]

LGTM

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/798873006/diff/60001/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:898: case ObjectLiteral::Property::PROTOTYPE:
{
On 2015/01/16 09:43:54, Michael Starzinger wrote:
> On 2015/01/15 16:51:02, arv wrote:
> > On 2015/01/15 16:35:11, rossberg wrote:
> > > On 2015/01/15 16:12:24, arv wrote:
> > > > On 2015/01/15 15:31:19, rossberg wrote:
> > > > > It seems to me that none but MATERIALIZED_LITERAL should possibly
appear
> > > here,
> > > > > so we might want to mark the others unreachable (similarly, in
> > > full-codegens).
> > > > 
> > > > I believe PROTOTYPE can happen too
> > > > 
> > > > class C {
> > > >   __proto__() {}
> > > > }
> > > 
> > > That is just considered a regular method definition, isn't it? As such, it
> > > shouldn't have PROTOTYPE kind AFAICT.
> > 
> > The AST only checks the value of the property name and since that matches
> > __proto__ it uses PROTOTYPE as the kind.
> > 
> > Let me refactor that in a different CL.
> 
> Done. Made CONSTANT and MATERIALIZED_LITERAL unreachable. Normally all
> FunctionLiterals should be COMPUTED, still need PROTOTYPE as per Arv's
> explanation. I left the full-codegens untouched.

I'll take care of this in a follow up CL