Change the HGraphBuilder to dispatch on the context.

Change the HGraphBuilder to dispatch on the context.

Before, expressions didn't take advantage of knowing their context in
the AST.  Now, we use the context to decide what to do with a value at
the end of visiting an expression.

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

[Kevin Millikin (Chromium), 2010-12-08 18:29:20]

Quick summary:  there are two new virtual member functions,
AstContext::ReturnValue and AstContext::ReturnInstruction. The first is used for
"pure" values that have already been emitted in the instruction stream (or don't
need to be) and had any side effects addressed, the second for instructions that
need to be emitted into the instruction stream and possibly have their
environment recorded.

They are called in tail position when visiting an instruction and passed the
value of the instruction.

Though I've mostly just moved code around (I was trying to preserve the existing
graph construction wherever possible), there are already some advantages.  I
have plans for more simplification and streamlining of the graph construction
process.

[Kevin Millikin (Chromium), 2010-12-09 08:23:23]

+kasperl

[Kasper Lund, 2010-12-09 11:40:08]

LGTM, but you may want to get Florian's opinion too.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode2063
src/hydrogen.cc:2063: HValue* true_value = invert_true()
Add a helper for this pattern. GetConstantBoolean(true|false).

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3227
src/hydrogen.cc:3227: {
Consider factoring the inner scope out into a separate named helper function.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3527
src/hydrogen.cc:3527: {
Helper?

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode3795
src/hydrogen.cc:3795: HSubgraph* subgraph = CreateBranchSubgraph(environment());
Helper? Maybe the three helpers can actually share something - not sure.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode4239
src/hydrogen.cc:4239: if (subgraph()->HasExit()) {
This code is duplicated below (along with a nice big comment). I would refactor
- maybe call it DoInline.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode563
src/hydrogen.h:563: // functions for expressions.
I guess you can only 'fill' a context once. Would it make sense to verify that
this is the way it is used by having a bool is_filled_ in debug mode?

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode580
src/hydrogen.h:580: int original_count_;
The comment mentions stack height. The name mentions count. Maybe somehow link
the two?

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode672
src/hydrogen.h:672: HEnvironment* environment() const { return
subgraph()->environment(); }
Long term maybe this should be Environment?

[fschneider, 2010-12-09 12:01:48]

LGTM. 

This looks much better!

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode3720
src/hydrogen.cc:3720: instr->set_position(expr->position());
It seems easy to forget setting the source position. Can the ReturnInstruction
take it as an argument? Or maybe can you easily assert inside ReturnInstruction
that all HInstructions that have require a position have it set?

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode4303
src/hydrogen.cc:4303: if (ast_context()->IsEffect()) AddSimulate(expr->id());
Shouldn't this be 

if (ast_context()->IsTest()) ...

then?

[Kevin Millikin (Chromium), 2010-12-09 12:45:54]

Thanks for the comments.  I'll hold off on trying to find the right abstractions
until I'm done deleting code.

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode3720
src/hydrogen.cc:3720: instr->set_position(expr->position());
On 2010/12/09 12:01:48, fschneider wrote:
> It seems easy to forget setting the source position. Can the ReturnInstruction
> take it as an argument? Or maybe can you easily assert inside
ReturnInstruction
> that all HInstructions that have require a position have it set?

I agree.  I removed one of the overloaded PushAndAdd functions because it was
easy to call the wrong one.

I don't have a good solution but I see the problem.  Hopefully when more code is
deleted we'll see a simple way to do it.

http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode4303
src/hydrogen.cc:4303: if (ast_context()->IsEffect()) AddSimulate(expr->id());
On 2010/12/09 12:01:48, fschneider wrote:
> Shouldn't this be 
> 
> if (ast_context()->IsTest()) ...
> 
> then?

Good catch.  It's the comment that's wrong.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode2063
src/hydrogen.cc:2063: HValue* true_value = invert_true()
On 2010/12/09 11:40:08, Kasper Lund wrote:
> Add a helper for this pattern. GetConstantBoolean(true|false).

I think I want to get rid of all occurrences of it with my next change, so I'll
leave it.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode563
src/hydrogen.h:563: // functions for expressions.
On 2010/12/09 11:40:08, Kasper Lund wrote:
> I guess you can only 'fill' a context once. Would it make sense to verify that
> this is the way it is used by having a bool is_filled_ in debug mode?

I'm not sure about that---it depends on how much state contexts hold.  For
instance, we might decide that the context surrounding 'e0 ? e1 : e2' gets
filled twice.

I'll hold off on asserting anything.

http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode672
src/hydrogen.h:672: HEnvironment* environment() const { return
subgraph()->environment(); }
On 2010/12/09 11:40:08, Kasper Lund wrote:
> Long term maybe this should be Environment?

Probably.  Or else we should get rid of the two or three layers of inlined
helpers, track the current running environment in the graph builder (there's no
great reason for it to be in the subgraph) and write directly:

env()->Push(...)

or whatever.