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#newcode1412 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#newcode453 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#newcode877 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#newcode931 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. http://codereview.chromium.org/7237024/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
