Collect type feedback in separate pass and store it in AST

Collect type feedback in separate pass and store it in AST

Notes:

- For now, just adds the missing type info fields to the AST nodes directly.
  I'd like to factor that out more nicely in a follow-up CL.

- All type feedback now is uniformly collected through AST nodes'
  RecordTypeFeedback functions. At some point, this logic should be moved
  out of ast.cc.

- The typing pass currently simulates the exact same conditions under
  which feedback was collected in Hydrogen before. That also should be
  made more generic in the future.

- Type information itself is unchanged. Making it more regular is
  yet more future work.

Some additional cleanups:

- Lifted out nested class ObjectLiteral::Property, to enable forward declaration.
- Moved around some auxiliary enums.


R=svenpanne@chromium.org
BUG=

Committed: http://code.google.com/p/v8/source/detail?r=14825

[Sven Panne, 2013-05-16 11:58:05]

https://codereview.chromium.org/14990014/diff/3001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/ast.cc#newcode421
src/ast.cc:421: for_in_type_ = static_cast<ForInType>(oracle->ForInType(this));
Hmmm, where should types like ForInType live in the long run? I don't have a
good idea yet, casts are :-P, circular dependencies are even more :-P

https://codereview.chromium.org/14990014/diff/3001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode376
src/ast.h:376: byte to_boolean_types() const { return to_boolean_types_; }
Do we still have problems with #include cycles, so we have to use "byte" here?
If not, the real type would be nicer.

https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode896
src/ast.h:896: ForInType for_in_type_;
Can we keep the fields and methods in a common order, e.g. syntactical parts
(i.e. the "real" AST children), then type annotations, and finally
bailout/feedback IDs? I don't care too much about the actual order, more about
the consistency. Separating the parts by empty lines or comments might be nice,
too.

https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc#newcode9783
src/hydrogen.cc:9783: ASSERT(feedback->IsSmi());
Looks a bit redundant now...

https://codereview.chromium.org/14990014/diff/3001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/typing.cc#newcode84
src/typing.cc:84: CHECK_ALIVE(VisitStatements(stmt->statements()));
Hmmm, do we really need a CHECK_ALIVE here? I thought we need this only for
early return. Anyway, can't we somehow have this centrally in AstVisitor::Visit?
There is already a check, but it is not 100% clear to me why that isn't enough.
Maybe monad by hand?

https://codereview.chromium.org/14990014/diff/3001/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/14990014/diff/3001/src/typing.h#newcode48
src/typing.h:48: void* operator new(size_t size, Zone* zone) {
What is the reason for the new/delete definitions here? If they are really
needed, a comment is appropriate. Furthermore, this is effectively making
AstTyper a ZoneObject (why?). Is it just avoiding multiple inheritance?

*sigh* In general, we should really templatize things more, this would just get
a ZoneAllocationPolicy then. We are extremely ad-hoc in such things. :-(

[rossberg, 2013-05-17 10:59:50]

https://codereview.chromium.org/14990014/diff/3001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/ast.cc#newcode421
src/ast.cc:421: for_in_type_ = static_cast<ForInType>(oracle->ForInType(this));
On 2013/05/16 11:58:05, Sven Panne wrote:
> Hmmm, where should types like ForInType live in the long run? I don't have a
> good idea yet, casts are :-P, circular dependencies are even more :-P

I don't have a good idea either. Probably will have to be some special type
domains on the side of 'real' types.

https://codereview.chromium.org/14990014/diff/3001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode376
src/ast.h:376: byte to_boolean_types() const { return to_boolean_types_; }
On 2013/05/16 11:58:05, Sven Panne wrote:
> Do we still have problems with #include cycles, so we have to use "byte" here?
> If not, the real type would be nicer.

Yes. But once the type representation has been unified, this problem should have
disappeared (because the low-level feedback types will be encapsulated in the
oracle).

https://codereview.chromium.org/14990014/diff/3001/src/ast.h#newcode896
src/ast.h:896: ForInType for_in_type_;
On 2013/05/16 11:58:05, Sven Panne wrote:
> Can we keep the fields and methods in a common order, e.g. syntactical parts
> (i.e. the "real" AST children), then type annotations, and finally
> bailout/feedback IDs? I don't care too much about the actual order, more about
> the consistency. Separating the parts by empty lines or comments might be
nice,
> too.

Done here and in a number of other places.

https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/hydrogen.cc#newcode9783
src/hydrogen.cc:9783: ASSERT(feedback->IsSmi());
On 2013/05/16 11:58:05, Sven Panne wrote:
> Looks a bit redundant now...

Indeed :)

https://codereview.chromium.org/14990014/diff/3001/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/14990014/diff/3001/src/typing.cc#newcode84
src/typing.cc:84: CHECK_ALIVE(VisitStatements(stmt->statements()));
On 2013/05/16 11:58:05, Sven Panne wrote:
> Hmmm, do we really need a CHECK_ALIVE here? I thought we need this only for
> early return. Anyway, can't we somehow have this centrally in
AstVisitor::Visit?
> There is already a check, but it is not 100% clear to me why that isn't
enough.
> Maybe monad by hand?

You have to check at every level to make failure abort, right? And yes, it's a
manual Maybe monad.

https://codereview.chromium.org/14990014/diff/3001/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/14990014/diff/3001/src/typing.h#newcode48
src/typing.h:48: void* operator new(size_t size, Zone* zone) {
On 2013/05/16 11:58:05, Sven Panne wrote:
> What is the reason for the new/delete definitions here? If they are really
> needed, a comment is appropriate. Furthermore, this is effectively making
> AstTyper a ZoneObject (why?). Is it just avoiding multiple inheritance?
> 
> *sigh* In general, we should really templatize things more, this would just
get
> a ZoneAllocationPolicy then. We are extremely ad-hoc in such things. :-(

This is simply analogous to the graph builder. Not sure I understand your
question, why would it not be zone-allocated?

[Sven Panne, 2013-05-17 11:29:26]

lgtm

[rossberg, 2013-05-27 13:59:43]

Committed patchset #4 manually as r14825 (presubmit successful).