Refactor handling of test expressions in the graph builder.

Refactor handling of test expressions in the graph builder.

Instead of generating two instructions and combining them
at lithium translation using the EmitAtUses predicate, we
generate the correct branch instruction right from the start.
Committed: http://code.google.com/p/v8/source/detail?r=8495

[fschneider, 2011-06-28 12:39:55]

This is the ia32 version. I'll start porting after the first round of feedback.

[fschneider, 2011-06-29 14:08:59]

Added the ports to all platforms.

[Kevin Millikin (Chromium), 2011-06-30 10:52:03]

It's nice to clean this up.  I've got a first round of comments.

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1412: return new
LCmpIDAndBranch(UseRegisterAtStart(left),
I prefer to name the UseRegisterXXX subexpressions in a local variable when
there is more than one.  Because they have subtle (and sometimes changing) side
effects, it's nice to make the register allocator evaluation-order independent.

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:453: return
HControlInstruction::cast(this->hydrogen_value());
this-> is needed here?

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:877: DECLARE_HYDROGEN_ACCESSOR(Test)
I intended to change the name of class HTest back to HBranch once there was no
confusion.  Is this a good time?

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:931: class HTest: public HUnaryControlInstruction {
I wonder if HUnaryControlInstruction is giving us much value at this point.  We
should strongly consider removing it.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2071
src/hydrogen.cc:2071: void EffectContext::ReturnInstruction(HInstruction* instr,
int ast_id) {
An alternative that I like better is to add a method void
AstContext::ReturnControl(HControlInstruction* instr, int ast_id) and avoid the
testing in these methods (even assert !instr->IsControlInstruction()).  I think
we should always know at the call site whether we have a control instruction or
not.  (Alternative names: ReturnSplit, ReturnBranch.)

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2077
src/hydrogen.cc:2077:
owner()->MaterializeBoolean(HControlInstruction::cast(instr), ast_id);
I wonder why it is necessary to materialize anything in an effect context?  I
can see needing the branch in our implementation, but not the Phi(true, false)
at the join.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2115
src/hydrogen.cc:2115: void TestContext::BuildBranch(HValue* value,
HControlInstruction* test) {
This is trying to do too much.  It's strange that it takes either NULL and a
control instruction or else a non-control instruction value and an HTest of that
same value.

I suggest restoring the original BuildBranch to branch on a value, and just
inlining the case of BuildBranch(NULL, instr) at the one call site.  You're not
really "building a branch" there anyway, you've already got a branch, so you're
building a join.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode5602
src/hydrogen.cc:5602: ast_context()->ReturnInstruction(result, expr->id());
Maybe we should take to writing

return ast_context()->ReturnInstruction(result, expr->id())

to make sure that they're in tail position :)

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode5664
src/hydrogen.cc:5664: void
HGraphBuilder::MaterializeBoolean(HControlInstruction* instr,
I find this function weird.  It should never be called for a test context.  For
an effect context, there should be not push in the branch target blocks (and
therefore no pop in the join block).

[fschneider, 2011-06-30 13:13:36]

Addressed first round of comments.

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:1412: return new
LCmpIDAndBranch(UseRegisterAtStart(left),
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> I prefer to name the UseRegisterXXX subexpressions in a local variable when
> there is more than one.  Because they have subtle (and sometimes changing)
side
> effects, it's nice to make the register allocator evaluation-order
independent.

I agree we should be consistent in this. It is a potential source of bugs in
combination with DefineSameAsFirst.

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:453: return
HControlInstruction::cast(this->hydrogen_value());
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> this-> is needed here?

Unfortunately GCC complains otherwise (or asks to use -fpermissive).

http://codereview.chromium.org/7237024/diff/16008/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:877: DECLARE_HYDROGEN_ACCESSOR(Test)
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> I intended to change the name of class HTest back to HBranch once there was no
> confusion.  Is this a good time?

Done.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:931: class HTest: public HUnaryControlInstruction {
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> I wonder if HUnaryControlInstruction is giving us much value at this point. 
We
> should strongly consider removing it

Not much, the helper value() for OperandAt(0) and the constructor. - It would
involve however changing a lot of lines of code removing it.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2071
src/hydrogen.cc:2071: void EffectContext::ReturnInstruction(HInstruction* instr,
int ast_id) {
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> An alternative that I like better is to add a method void
> AstContext::ReturnControl(HControlInstruction* instr, int ast_id) and avoid
the
> testing in these methods (even assert !instr->IsControlInstruction()).  I
think
> we should always know at the call site whether we have a control instruction
or
> not.  (Alternative names: ReturnSplit, ReturnBranch.)

Done.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2077
src/hydrogen.cc:2077:
owner()->MaterializeBoolean(HControlInstruction::cast(instr), ast_id);
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> I wonder why it is necessary to materialize anything in an effect context?  I
> can see needing the branch in our implementation, but not the Phi(true, false)
> at the join.

Done. Removed MaterializeBoolean.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode2115
src/hydrogen.cc:2115: void TestContext::BuildBranch(HValue* value,
HControlInstruction* test) {
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> This is trying to do too much.  It's strange that it takes either NULL and a
> control instruction or else a non-control instruction value and an HTest of
that
> same value.
> 
> I suggest restoring the original BuildBranch to branch on a value, and just
> inlining the case of BuildBranch(NULL, instr) at the one call site.  You're
not
> really "building a branch" there anyway, you've already got a branch, so
you're
> building a join.

Done.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode5602
src/hydrogen.cc:5602: ast_context()->ReturnInstruction(result, expr->id());
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> Maybe we should take to writing
> 
> return ast_context()->ReturnInstruction(result, expr->id())
> 
> to make sure that they're in tail position :)

Done.

http://codereview.chromium.org/7237024/diff/16008/src/hydrogen.cc#newcode5664
src/hydrogen.cc:5664: void
HGraphBuilder::MaterializeBoolean(HControlInstruction* instr,
On 2011/06/30 10:52:03, Kevin Millikin wrote:
> I find this function weird.  It should never be called for a test context. 
For
> an effect context, there should be not push in the branch target blocks (and
> therefore no pop in the join block).

Removed.

[Kevin Millikin (Chromium), 2011-06-30 13:31:05]

LGTM.

http://codereview.chromium.org/7237024/diff/14021/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7237024/diff/14021/src/hydrogen.cc#newcode2118
src/hydrogen.cc:2118: ReturnValue(owner()->Pop());
No need for this last line.  All it does is check Phi(true, false) to make sure
it's not the arguments value and then pushes it back on the environment.

http://codereview.chromium.org/7237024/diff/14021/src/hydrogen.cc#newcode3006
src/hydrogen.cc:3006: return ast_context()->ReturnInstruction(instr,
expr->id());
:)

http://codereview.chromium.org/7237024/diff/14021/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/7237024/diff/14021/src/hydrogen.h#newcode501
src/hydrogen.h:501: virtual void ReturnControl(HControlInstruction* instr, int
ast_id) = 0;
Needs a comment in similar detail to ReturnValue and ReturnInstruction.