First steps towards better code generation for LBranch:

First steps towards better code generation for LBranch:

 * AST Expression nodes get a separate testing ID to record type info in
   ToBooleanStub later. This is necessary to avoid clashes with other uses of
   already existing IDs.

 * In order to avoid threading the condition expression through tons of places,
   TestContexts carry it now with them. Note that we will probably only need the
   testing ID of the expression, but having the whole thing at hand makes
   debugging easier. Probably we will change this later...
Committed: http://code.google.com/p/v8/source/detail?r=8274

[Sven Panne, 2011-06-09 10:07:56]

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

http://codereview.chromium.org/7046073/diff/1/src/hydrogen.cc#newcode1991
src/hydrogen.cc:1991: test_context_ = new TestContext(owner, cond, if_true,
if_false);
Is this part of the change correct? I'm only 50% sure... :-/

[fschneider, 2011-06-10 09:18:18]

LGTM as a first step.

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc#newcode747
src/full-codegen.cc:747: DoTest(expr, &discard, &restore, &restore);
Shouldn't the test of the left subexpressions get the test ID left->test_id()?
It has to match what the Crankshaft graph builder is doing.

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

http://codereview.chromium.org/7046073/diff/1/src/hydrogen.cc#newcode1991
src/hydrogen.cc:1991: test_context_ = new TestContext(owner, cond, if_true,
if_false);
On 2011/06/09 10:07:57, Sven wrote:
> Is this part of the change correct? I'm only 50% sure... :-/

You probably want the call-instruction as the condition here.
Inlined functions may be a little tricky, since each return-statement will
become a HTest. I guess it is fine to use the type feedback (and test_id) of the
call-instruction. Not sure if specializing each return-site would help much.

[Kevin Millikin (Chromium), 2011-06-10 09:35:06]

LGTM if you use the left subexpression ID for the left subexpression of logical
&& and ||.

http://codereview.chromium.org/7046073/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7046073/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:386: codegen()->DoTest(condition_, true_label_,
false_label_, fall_through_);
Hmmm, it might be useful to have a DoTest that takes a test context (and just
forwards to the one with explicit arguments) for simplicity.

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc#newcode747
src/full-codegen.cc:747: DoTest(expr, &discard, &restore, &restore);
On 2011/06/10 09:18:18, fschneider wrote:
> Shouldn't the test of the left subexpressions get the test ID left->test_id()?
> It has to match what the Crankshaft graph builder is doing.

I think you could do it either way (as long as they agree), but Florian's
suggestions seems more straightforward.  Use the ID of the expression we're
testing.

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

http://codereview.chromium.org/7046073/diff/1/src/hydrogen.cc#newcode1991
src/hydrogen.cc:1991: test_context_ = new TestContext(owner, cond, if_true,
if_false);
On 2011/06/10 09:18:18, fschneider wrote:
> On 2011/06/09 10:07:57, Sven wrote:
> > Is this part of the change correct? I'm only 50% sure... :-/
> 
> You probably want the call-instruction as the condition here.
> Inlined functions may be a little tricky, since each return-statement will
> become a HTest. I guess it is fine to use the type feedback (and test_id) of
the
> call-instruction. Not sure if specializing each return-site would help much.

I don't think you can specialize the individual return sites, since we only have
the call in unoptimized code.  This seems right (using the call expression,
since you really only need the ID).

[Sven Panne, 2011-06-14 08:04:48]

http://codereview.chromium.org/7046073/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7046073/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:386: codegen()->DoTest(condition_, true_label_,
false_label_, fall_through_);
On 2011/06/10 09:35:06, Kevin Millikin wrote:
> Hmmm, it might be useful to have a DoTest that takes a test context (and just
> forwards to the one with explicit arguments) for simplicity.

I think it even makes sense to have *only* such a DoTest. This has the advantage
that the fact will be very explicit that we use the value of the left operand
for *2* things: The value itself and for control.

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7046073/diff/1/src/full-codegen.cc#newcode747
src/full-codegen.cc:747: DoTest(expr, &discard, &restore, &restore);
On 2011/06/10 09:35:06, Kevin Millikin wrote:
> On 2011/06/10 09:18:18, fschneider wrote:
> > Shouldn't the test of the left subexpressions get the test ID
left->test_id()?
> > It has to match what the Crankshaft graph builder is doing.
> 
> I think you could do it either way (as long as they agree), but Florian's
> suggestions seems more straightforward.  Use the ID of the expression we're
> testing.

Done.