Move AST node counting to post-pass

Move AST node counting to post-pass

R=mstarzinger@chromium.org, svenpanne@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=24909

[wingo, 2014-10-22 15:29:37]

Patchset #1 (id:1) has been deleted

[wingo, 2014-10-22 15:33:44]

Patchset #1 (id:20001) has been deleted

[wingo, 2014-10-22 15:35:26]

This CL also consolidates the AST transformations in compiler.{h,cc}; let me
know what you think.

[wingo, 2014-10-23 13:53:12]

A gentle ping :)  In the confusion of multiple patches, it can be hard to
remember where to start; it's here :)

[Sven Panne, 2014-10-24 08:25:12]

lgtm

https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc#newcode931
src/compiler.cc:931: if (!Renumber(&unoptimized)) return false;
Why do we need that here, i.e. what is the path leading to that point where we
have an "un-numbered" AST?

[Sven Panne, 2014-10-24 08:26:49]

On 2014/10/24 08:25:12, Sven Panne wrote:
> lgtm
> 
> https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc#newcode931
> src/compiler.cc:931: if (!Renumber(&unoptimized)) return false;
> Why do we need that here, i.e. what is the path leading to that point where we
> have an "un-numbered" AST?

Ooops, klicked LGTM instead of just "Publish". Things look OK, but before we
land this, I'd like to understand the point above. Another related question: Is
there a path where we Renumber() twice? It's a bit hard to see (but that's not
your fault).

[wingo, 2014-10-24 08:59:29]

On 2014/10/24 08:25:12, Sven Panne wrote:
> lgtm
> 
> https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/675493002/diff/40001/src/compiler.cc#newcode931
> src/compiler.cc:931: if (!Renumber(&unoptimized)) return false;
> Why do we need that here, i.e. what is the path leading to that point where we
> have an "un-numbered" AST?

You know, I don't know.  I was thinking about that myself.  I'll investigate.

[wingo, 2014-10-24 09:25:39]

New patchset removes needless Renumber(), which AFAICS is always dominated by
another Renumber() call, so it's idempotent.  Passes tests.

[wingo, 2014-10-24 09:34:01]

On 2014/10/24 08:26:49, Sven Panne wrote:
> Is
> there a path where we Renumber() twice? It's a bit hard to see (but that's not
> your fault).

I don't really know.  There are two possible cases:

 1. Any time you call Analyze() instead of ParseAndAnalyze()

 2. Any time you call Renumber()

(1) only happens in CompileUnoptimized code AFAICS; anyway this is the same
state of things as before, as it also calls Rewrite() and Scope::Analyze().  I
can't convince myself that duplicate calls are impossible, but since Rewrite /
Scope::Analyze can change the AST we have to renumber there

(2) happens in BuildFunctionInfo, which usually happens when visiting a function
via AST traversal.  If the FunctionLiteral survives traversal (to then be
traversed again) then I guess it's possible, but unlikely; it would only happen
for parenthesized functions.

So in summary I don't really know :)

[Sven Panne, 2014-10-24 09:39:10]

LGTM. The actual counting part looks OK, and the "pipeline" is improved a bit,
although there seems to be nobody completely understanding it... :-}

[Sven Panne, 2014-10-24 10:39:22]

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

[Sven Panne, 2014-10-24 10:40:09]

Added Michael, the "pipeline" is complicated enough to warrant a 2nd reviewer.
:-}

[wingo, 2014-10-27 07:44:31]

On 2014/10/24 10:40:09, Sven Panne wrote:
> Added Michael, the "pipeline" is complicated enough to warrant a 2nd reviewer.
> :-}

I am always happy to have the opportunity to get review from Michael :)

[Michael Starzinger, 2014-10-27 11:41:54]

LGTM, just nitty suggestions.

https://codereview.chromium.org/675493002/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/675493002/diff/60001/src/compiler.cc#newcode922
src/compiler.cc:922: bool Compiler::EnsureDeoptimizationSupport(CompilationInfo*
info) {
nit: We might even want to add a two assertions here ...

DCHECK(info->function() != NULL);
DCHECK(info->scope() != NULL);

https://codereview.chromium.org/675493002/diff/60001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/675493002/diff/60001/src/compiler.h#newcode686
src/compiler.h:686: static bool EnsureDeoptimizationSupport(CompilationInfo*
info);
nit: I know it's not part of you change, but can we also add a comment here
(above EnsureDeoptimizationSupport) saying that ParseAndAnalyze needs to have
been called before? Something along the lines of ...

// Adds deoptimization support, requires ParseAndAnalyze.

Also it might make sense to move it up to right after Analyze(), to make clear
that it is (optionally) part of the pipeline.

[wingo, 2014-10-27 14:58:10]

Thanks for the review -- fixed nits and landing

https://codereview.chromium.org/675493002/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/675493002/diff/60001/src/compiler.cc#newcode922
src/compiler.cc:922: bool Compiler::EnsureDeoptimizationSupport(CompilationInfo*
info) {
On 2014/10/27 11:41:54, Michael Starzinger wrote:
> nit: We might even want to add a two assertions here ...
> 
> DCHECK(info->function() != NULL);
> DCHECK(info->scope() != NULL);

Done.

https://codereview.chromium.org/675493002/diff/60001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/675493002/diff/60001/src/compiler.h#newcode686
src/compiler.h:686: static bool EnsureDeoptimizationSupport(CompilationInfo*
info);
On 2014/10/27 11:41:54, Michael Starzinger wrote:
> nit: I know it's not part of you change, but can we also add a comment here
> (above EnsureDeoptimizationSupport) saying that ParseAndAnalyze needs to have
> been called before? Something along the lines of ...
> 
> // Adds deoptimization support, requires ParseAndAnalyze.
> 
> Also it might make sense to move it up to right after Analyze(), to make clear
> that it is (optionally) part of the pipeline.

Done.

[wingo, 2014-10-27 15:00:33]

Committed patchset #3 (id:80001) manually as 24909 (presubmit successful).