Thanks for submitting this CL. I have some drive by comments below.
http://codereview.chromium.org/8857001/diff/17002/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/8857001/diff/17002/src/arm/lithium-codegen-arm.cc#newcode2311 src/arm/lithium-codegen-arm.cc:2311: } else { You can do better here and use a conditional move instead: __ mov(result, Operand(factory()->undefined_value()), LeaveCC, eq); http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h#newcode3549 src/hydrogen-instructions.h:3549: return mode_ == kCheckIgnoreAssignment; This should better be mode_ != kNoCheck? http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode3821 src/hydrogen.cc:3821: Bind(var, Top()); mjsunit/compiler/regress-const.js fails because of a compound assignment to a stack allocated CONST variable. You should also skip the assignment here. http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode3845 src/hydrogen.cc:3845: case CONST_HARMONY: You can put an UNREACHABLE() in the CONST_HARMONY case, because we statically check for this case and throw an early syntax error. http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode4035 src/hydrogen.cc:4035: case CONST_HARMONY: Here also CONST_HARMONY should be UNREACHABLE(). http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode4047 src/hydrogen.cc:4047: ASSERT(expr->op() == Token::INIT_CONST_HARMONY || Here CONST_HARMONY can be moved to the HStoreContextSlot::kNoCheck group, because in contrast to CONST repeated initializations (for example in a loop) are not possible with CONST_HARMONY. http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode5624 src/hydrogen.cc:5624: return Bailout("unsupported count operation with const"); It should be trivial to also remove this bailout and do the same thing as in the CompoundAssignments and ignore the assignment to CONST. Could you also add simple test cases, that test that crankshafted CompoundAssignments and CountOperations have the expected behaviour for context allocated CONST bindings? http://codereview.chromium.org/8857001/diff/17002/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/8857001/diff/17002/src/ia32/lithium-codegen-ia32.cc#newcode2176 src/ia32/lithium-codegen-ia32.cc:2176: Those blank lines do not help readability. Could you remove them please? http://codereview.chromium.org/8857001/diff/17002/src/mips/lithium-codegen-mips.cc File src/mips/lithium-codegen-mips.cc (right): http://codereview.chromium.org/8857001/diff/17002/src/mips/lithium-codegen-mips.cc#newcode2188 src/mips/lithium-codegen-mips.cc:2188: } else { You can also introduce a conditional move here. Compare LCodeGen::DoArgumentsElements on arm and mips to see how to do it. http://codereview.chromium.org/8857001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
