AstNumberingVisitor does the work of AstConstructionVisitor

AstNumberingVisitor does the work of AstConstructionVisitor

R=svenpanne@chromium.org
BUG=

[wingo, 2014-10-22 10:44:53]

Here's a patch to simplify the parser by moving AstConstructionVisitor's work
to a post-pass.

The AstConstructionVisitor computes the following things:

  1. a cumulative ast node count for a function
  2. the kDontSelfOptimize / kDontSoftInline / kDontCache flags
  3. the feedback / ic feedback slots.
  4. the dont_crankshaft_reason / dont_turbofan_reason bailoutreasons.

Ultimately this information propagates to the FunctionLiteral and thence
to the SharedFunctionInfo.

An observation: all of this information is only needed once we've made
the commitment to full-codegen a function, and thus have run the
post-pass.

  1. The ast node count is used in inlining decisions.  I think it used
     to be used in optimization decisions but with asm.js that's no
     longer the case.  In any case when we go to inline a function, it's
     probably been called before, and thus the ast node count has
     already been set on the sharedfunctioninfo when the function was
     parsed/full-codegenned.

     * Question: is this correct?  Can we inline functions that haven't
       been called before?

  2. The flags are all used after full-codegen, (kDontSoftInline is unused)

  3. Likewise for the feedback slot counts and allocations.

  4. Likewise for the bailoutreasons.

The patch is straightforward, except for a couple open questions:

 1) Does this affect inlining / optimization decisions?  I think not but I don't
know

 2) Is it OK for a lazy function to have an empty type feedback vector, which
later gets reallocated?  Is the liveedit treatment of type feedback vectors
correct?

[wingo, 2014-10-22 10:54:20]

x64.release d8 size with this patch is down by 0.15%.  Instructions retired when
running on jquery-parse-test.js down by 1.28% (--no-lazy), or 0.17% (--lazy).

It would appear to be a minor win on startup, then.

[mvstanton, 2014-10-22 12:12:03]

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

[mvstanton, 2014-10-22 12:12:04]

DBC: In general I like the idea of doing the feedback vector creation after
scoping because with --vector-ics turned on too many slots are being wasted. So
+1 on that. I think the live-edit stuff might be in trouble though...do those
tests pass?

One CL per work item that moves to the new phase might be good as well in case a
problem is found with just one of them. Of course, you don't get the full joy of
tearing down old structures until they are all done!

All the best,
--Michael

[wingo, 2014-10-22 12:46:40]

On 2014/10/22 12:12:04, mvstanton wrote:
> DBC: In general I like the idea of doing the feedback vector creation after
> scoping because with --vector-ics turned on too many slots are being wasted.
So
> +1 on that. I think the live-edit stuff might be in trouble though...do those
> tests pass?

"make x64.release.check" and "make x64.debug.check" both pass, which do include
liveedit tests; I haven't done any other kind of testing though.  I did change
one liveedit test, though, to not expect feedback vectors to be present for
functions that aren't compiled.

[Sven Panne, 2014-10-22 13:08:52]

Can we split up this CL into several ones? As you've already mentioned, it
consists of 4 parts plus actually removing AstConstructionVisitor. Having
several CLs is a bit more tedious, but much easier to review (some CLs are
probably easy and non-controversial), debug and analyze if there are any
regressions.

https://codereview.chromium.org/660083005/diff/1/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/660083005/diff/1/src/ast-numbering.cc#newcode559
src/ast-numbering.cc:559: Handle<SharedFunctionInfo> shared, Zone* zone) {
I don't think this actually belongs here: It just copies from the
FunctionLiteral to the SharedFunctionInfo. Perhaps this should live in
SharedFunctionInfo or even in some yet-to-be-implemented part of the compilation
pipeline (frontend). Anyway, having another Renumber method is just confusing.

[wingo, 2014-10-22 13:23:02]

ACK regarding splitting the patchset.

https://codereview.chromium.org/660083005/diff/1/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/660083005/diff/1/src/ast-numbering.cc#newcode559
src/ast-numbering.cc:559: Handle<SharedFunctionInfo> shared, Zone* zone) {
On 2014/10/22 13:08:52, Sven Panne wrote:
> I don't think this actually belongs here: It just copies from the
> FunctionLiteral to the SharedFunctionInfo. Perhaps this should live in
> SharedFunctionInfo or even in some yet-to-be-implemented part of the
compilation
> pipeline (frontend). Anyway, having another Renumber method is just confusing.

ACK.  Unfortunately the fix is a bit more involved, as this copying usually
should happen after Renumber is run, but Renumber is run from a few places, so
we need a central place to call Renumber -- yet-to-be-implemented, as you say.

[wingo, 2014-10-23 07:13:07]

Closing issue, as I'm splitting it into pieces.