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