LGTM with nits :-) And a handful of real comments too.
https://codereview.chromium.org/10701054/diff/67140/Makefile File Makefile (right): https://codereview.chromium.org/10701054/diff/67140/Makefile#newcode84 Makefile:84: # vfp2=on nit: the comment is supposed to indicate the explicit flag being handled here, i.e. the non-default value (see examples above and below). https://codereview.chromium.org/10701054/diff/67140/src/arm/assembler-arm.h File src/arm/assembler-arm.h (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/assembler-arm.h#newcode264 src/arm/assembler-arm.h:264: DwVfpRegister r = DwVfpRegister(code); nit: how about just: return DwVfpRegister(code); https://codereview.chromium.org/10701054/diff/67140/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/code-stubs-arm.cc#newcode41 src/arm/code-stubs-arm.cc:41: KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) { nit: indentation please https://codereview.chromium.org/10701054/diff/67140/src/arm/code-stubs-arm.cc#newcode3808 src/arm/code-stubs-arm.cc:3808: // regenerate, which leads to code stub initialization state being messed up. s/leads/would lead/? https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-arm.h File src/arm/lithium-arm.h (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-arm.h#newcode260 src/arm/lithium-arm.h:260: bool ClobbersDoubleRegisters() const { return is_call_; } Why doesn't this need " || !CpuFeatures::IsSupported(VFP2);" as ia32 does? https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode291 src/arm/lithium-codegen-arm.cc:291: Comment(";;; Deferred destory frame", nit: "destroy" https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode312 src/arm/lithium-codegen-arm.cc:312: // Check that the jump table is acvcessible from everywhere in the function nit: I prefer the old version. https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode328 src/arm/lithium-codegen-arm.cc:328: bool has_generated_needs_frame_not_call = false; see comments on ia32 version. In short: 1) if you use the Label::is_bound() method, you don't need the separate bool. 2) I'd reorder the blocks to "if (bound) { jump } else { bind }" 3) comments are weird https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode1451 src/arm/lithium-codegen-arm.cc:1451: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed, as the Scope c'tor right below contains the same ASSERT. https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode1738 src/arm/lithium-codegen-arm.cc:1738: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode1908 src/arm/lithium-codegen-arm.cc:1908: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode1955 src/arm/lithium-codegen-arm.cc:1955: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode2047 src/arm/lithium-codegen-arm.cc:2047: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode2214 src/arm/lithium-codegen-arm.cc:2214: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3700 src/arm/lithium-codegen-arm.cc:3700: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3738 src/arm/lithium-codegen-arm.cc:3738: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3765 src/arm/lithium-codegen-arm.cc:3765: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3832 src/arm/lithium-codegen-arm.cc:3832: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3841 src/arm/lithium-codegen-arm.cc:3841: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3864 src/arm/lithium-codegen-arm.cc:3864: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3898 src/arm/lithium-codegen-arm.cc:3898: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode3978 src/arm/lithium-codegen-arm.cc:3978: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4269 src/arm/lithium-codegen-arm.cc:4269: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4341 src/arm/lithium-codegen-arm.cc:4341: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4619 src/arm/lithium-codegen-arm.cc:4619: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4638 src/arm/lithium-codegen-arm.cc:4638: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4726 src/arm/lithium-codegen-arm.cc:4726: Operand(hiword, LSL, mantissa_shift_for_hi_word)); I can't convince myself that this is right. I think it should be either LSR, or shift by -mantissa_shift_for_hi_word. AFAICS this "else" branch is currently dead code, because |leading_zeroes| is either 0 or 1, so mantissa_shift_for_hi_word is always positive (either 10 or 11). We should either guard this with an ASSERT, or fix it before someone uses this code differently :-) https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4904 src/arm/lithium-codegen-arm.cc:4904: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode4980 src/arm/lithium-codegen-arm.cc:4980: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode5222 src/arm/lithium-codegen-arm.cc:5222: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode5232 src/arm/lithium-codegen-arm.cc:5232: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.cc#newcode5241 src/arm/lithium-codegen-arm.cc:5241: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.h File src/arm/lithium-codegen-arm.h (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-arm.h#newcode382 src/arm/lithium-codegen-arm.h:382: explicit inline JumpTableEntry(Address entry, bool frame, bool is_lazy) nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc File src/arm/lithium-gap-resolver-arm.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode174 src/arm/lithium-gap-resolver-arm.cc:174: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode178 src/arm/lithium-gap-resolver-arm.cc:178: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode199 src/arm/lithium-gap-resolver-arm.cc:199: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode203 src/arm/lithium-gap-resolver-arm.cc:203: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode240 src/arm/lithium-gap-resolver-arm.cc:240: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode280 src/arm/lithium-gap-resolver-arm.cc:280: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resolver-arm.cc#newcode291 src/arm/lithium-gap-resolver-arm.cc:291: ASSERT(CpuFeatures::IsSupported(VFP2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/ast.h File src/ast.h (right): https://codereview.chromium.org/10701054/diff/67140/src/ast.h#newcode2515 src/ast.h:2515: public: \ nit: indentation https://codereview.chromium.org/10701054/diff/67140/src/ast.h#newcode2531 src/ast.h:2531: private: \ nit: indentation https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc File src/code-stubs-hydrogen.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc#newcode53 src/code-stubs-hydrogen.cc:53: explicit CodeStubGraphBuilderBase(Isolate* isolate, nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc#newcode57 src/code-stubs-hydrogen.cc:57: protected: nit: empty line before visibility sections (doesn't presubmit.py complain about this?) https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc#newcode61 src/code-stubs-hydrogen.cc:61: private: nit: empty line before visibility sections https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc#newcode80 src/code-stubs-hydrogen.cc:80: CodeStubInterfaceDescriptor** decsriptors = nit: descriptors https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.cc#newcode112 src/code-stubs-hydrogen.cc:112: private: nit: don't need an empty "private:" section https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.cc#newcode102 src/code-stubs.cc:102: GetICState()); nit: fits on previous line? https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h File src/code-stubs.h (right): https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode173 src/code-stubs.h:173: friend class PlatformCodeStub; Since both of these friends are also subclasses, why not make the stuff they need protected? If you do keep the "friend class" approach, please merge this section with "friend class BreakPointIterator" below. https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode247 src/code-stubs.h:247: struct CodeStubInterfaceDescriptor { Nice! https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1058 src/code-stubs.h:1058: explicit KeyedLoadDictionaryElementStub() {} nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1072 src/code-stubs.h:1072: explicit KeyedLoadFastElementStub(bool is_js_array, nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1074 src/code-stubs.h:1074: bit_field_ = ElementsKindBits::encode(elements_kind); nit: elsewhere, we use the pattern: bit_field_ = Foo::encode(foo) | Bar::encode(bar); https://codereview.chromium.org/10701054/diff/67140/src/codegen.cc File src/codegen.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/codegen.cc#newcode150 src/codegen.cc:150: code->Disassemble(""); use info->stub()->Major()? https://codereview.chromium.org/10701054/diff/67140/src/deoptimizer.cc File src/deoptimizer.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/deoptimizer.cc#newcode80 src/deoptimizer.cc:80: nit: why remove the second empty line? https://codereview.chromium.org/10701054/diff/67140/src/full-codegen.h File src/full-codegen.h (right): https://codereview.chromium.org/10701054/diff/67140/src/full-codegen.h#newcode68 src/full-codegen.h:68: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS(); Ugh. I didn't realize immediately how many AstVisitor subclasses we have. We should revisit the way how we handle these methods (but not in this CL). https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode628 src/hydrogen.cc:628: { why this block? Shouldn't make a difference if you just remove the braces. https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode3326 src/hydrogen.cc:3326: Expression* expr, nit: line break before first argument not necessary? https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode4895 src/hydrogen.cc:4895: HOptimizedGraphBuilder::LookupGlobalProperty( nit: I think I'd indent this line (by 4 spaces; and then the next by 4 more). https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode9818 src/hydrogen.cc:9818: PrintStringProperty("name", "stub"); Can we print info->stub()->major_key() or something? https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.h File src/hydrogen.h (right): https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.h#newcode714 src/hydrogen.h:714: explicit ValueContext(HOptimizedGraphBuilder* owner, nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.h#newcode1304 src/hydrogen.h:1304: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS(); These used to be public; did you move them to the private section intentionally? https://codereview.chromium.org/10701054/diff/67140/src/ia32/assembler-ia32.h File src/ia32/assembler-ia32.h (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/assembler-ia32.h#newcode169 src/ia32/assembler-ia32.h:169: bool is(IntelDoubleRegister reg) const { return code_ == reg.code_; } Wouldn't this return xmm0.is(x87tos) == true? Is that intentional? https://codereview.chromium.org/10701054/diff/67140/src/ia32/assembler-ia32.h#newcode185 src/ia32/assembler-ia32.h:185: XMMRegister r = XMMRegister(code); just "return XMMRegister(code);"? https://codereview.chromium.org/10701054/diff/67140/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/code-stubs-ia32.cc#newcode45 src/ia32/code-stubs-ia32.cc:45: KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) { nit: indentation please https://codereview.chromium.org/10701054/diff/67140/src/ia32/code-stubs-ia32.cc#newcode47 src/ia32/code-stubs-ia32.cc:47: static CodeStubInterfaceDescriptor* result; I'd suspect at least one of the common compilers to complain about "...may be used uninitialized" here. If you initialized |result| to NULL, wouldn't that also mean you could get by without |initialized| by simply checking "if (result == NULL) {" below? (The same question applies to arm/x64.) https://codereview.chromium.org/10701054/diff/67140/src/ia32/deoptimizer-ia32.cc File src/ia32/deoptimizer-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/deoptimizer-ia32.cc#newcode595 src/ia32/deoptimizer-ia32.cc:595: Code* continuation = Please choose another name either for this or for the "JSFunction continuation" in the diagram above. (Same on the other platforms.) https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode199 src/ia32/lithium-codegen-ia32.cc:199: __ push(esi); nit: could hoist this out of the if/else blocks. https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode364 src/ia32/lithium-codegen-ia32.cc:364: if (!has_generated_needs_frame_is_call) { I think you can replace the check for !has_generated_needs_frame_is_call with !needs_frame_is_call.is_bound(), eliminating the need to have an extra bool for each label. Also, I think readability would improve a bit (fewer double negations) if you reordered the two blocks: if (label.is_bound()) { __ jmp(&label); } else { __ bind(&label); // ... } https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode368 src/ia32/lithium-codegen-ia32.cc:368: // If there is not frame, we don't have access to the JSFunction that "there is not frame" has not grammar. Aside from that, of course there is no frame, we're just building one. Did you mean "When we're compiling a stub, ..."? https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode389 src/ia32/lithium-codegen-ia32.cc:389: if (!has_generated_needs_frame_not_call) { same here https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode393 src/ia32/lithium-codegen-ia32.cc:393: // If there is not frame, we don't have access to the JSFunction that see above. https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode443 src/ia32/lithium-codegen-ia32.cc:443: Comment(";;; Deferred destory frame", nit: "destroy" https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode1604 src/ia32/lithium-codegen-ia32.cc:1604: ASSERT(CpuFeatures::IsSupported(SSE2)); nit: not needed. The Scope c'tor right below contains the same ASSERT. https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode1821 src/ia32/lithium-codegen-ia32.cc:1821: ASSERT(CpuFeatures::IsSupported(SSE2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode3833 src/ia32/lithium-codegen-ia32.cc:3833: ASSERT(CpuFeatures::IsSupported(SSE2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode5063 src/ia32/lithium-codegen-ia32.cc:5063: ASSERT(CpuFeatures::IsSupported(SSE2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.cc#newcode5079 src/ia32/lithium-codegen-ia32.cc:5079: ASSERT(CpuFeatures::IsSupported(SSE2)); nit: not needed https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.h File src/ia32/lithium-codegen-ia32.h (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-ia32.h#newcode373 src/ia32/lithium-codegen-ia32.h:373: explicit inline JumpTableEntry(Address entry, bool frame, bool is_lazy) nit: no need for "explicit" https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-gap-resolver-ia32.cc File src/ia32/lithium-gap-resolver-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-gap-resolver-ia32.cc#newcode1 src/ia32/lithium-gap-resolver-ia32.cc:1: // Copyright 2012 the V8 project authors. All rights reserved. nit: no need for this change https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-ia32.cc File src/ia32/lithium-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-ia32.cc#newcode823 src/ia32/lithium-ia32.cc:823: if (graph()->info()->IsOptimizing()) { I guess the changes to this method need porting (well, copying) to the other platforms. Currently, the implementations of LChunkBuilder::DoBasicBlock() are identical on ia32/x64/arm, so I guess the changes apply everywhere too. https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-ia32.h#newcode1431 src/ia32/lithium-ia32.h:1431: return !IsDoubleOrFloatElementsKind(hydrogen()->elements_kind()); Why the "!"? Doesn't it clobber the x87tos register precisely when the elements_kind *is* a floating-point kind? https://codereview.chromium.org/10701054/diff/67140/src/ia32/macro-assembler-ia32.cc File src/ia32/macro-assembler-ia32.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ia32/macro-assembler-ia32.cc#newcode1804 src/ia32/macro-assembler-ia32.cc:1804: CEntryStub ces(1, CpuFeatures::IsSupported(SSE2) nit: indentation / line break. CEntryStub ces(1, CpuFeatures::IsSupported(SSE2) ? kSaveFPRegs : kDontSaveFPRegs); https://codereview.chromium.org/10701054/diff/67140/src/ic.cc File src/ic.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/ic.cc#newcode1060 src/ic.cc:1060: elements_kind).GetCode(); nit: fits on one line https://codereview.chromium.org/10701054/diff/67140/src/lithium-allocator.cc File src/lithium-allocator.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/lithium-allocator.cc#newcode1331 src/lithium-allocator.cc:1331: } Again, it might be mighty helpful to print *chunk_->info()->stub()->major_key() in the "else" case. https://codereview.chromium.org/10701054/diff/67140/src/mksnapshot.cc File src/mksnapshot.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/mksnapshot.cc#newcode1 src/mksnapshot.cc:1: // Copyright 2012 the V8 project authors. All rights reserved. nit: unnecessary change. Oh well. https://codereview.chromium.org/10701054/diff/67140/src/safepoint-table.cc File src/safepoint-table.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/safepoint-table.cc#newcode1 src/safepoint-table.cc:1: // Copyright 2012 the V8 project authors. All rights reserved. nit: unnecessary change https://codereview.chromium.org/10701054/diff/67140/src/smart-pointers.h File src/smart-pointers.h (right): https://codereview.chromium.org/10701054/diff/67140/src/smart-pointers.h#newcode66 src/smart-pointers.h:66: // You can use [n] to index as if it was a plain pointer nit: trailing full stop please. https://codereview.chromium.org/10701054/diff/67140/src/spaces.cc File src/spaces.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/spaces.cc#newcode1 src/spaces.cc:1: // Copyright 2012 the V8 project authors. All rights reserved. nit: unnecessary change https://codereview.chromium.org/10701054/diff/67140/src/x64/assembler-x64.cc File src/x64/assembler-x64.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/x64/assembler-x64.cc#newcode205 src/x64/assembler-x64.cc:205: Register::kRegisterCodeByAllocationIndex[kMaxNumAllocatableRegisters] = { nit: indentation please https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc File src/x64/builtins-x64.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc#newcode654 src/x64/builtins-x64.cc:654: // Preserve registers across notification this is important compiled stubs nit: missing comma after "notification", important *for* compiled stubs https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc#newcode674 src/x64/builtins-x64.cc:674: // Preserve registers across notification, this is important compiled stubs nit: important *for* compiled stubs https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc#newcode677 src/x64/builtins-x64.cc:677: __ Pushad(); Why do the other platforms (both ia32 and ARM) not have to do this? Copy/paste error? https://codereview.chromium.org/10701054/diff/67140/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/x64/code-stubs-x64.cc#newcode41 src/x64/code-stubs-x64.cc:41: KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) { nit: indentation please https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.cc File src/x64/lithium-codegen-x64.cc (right): https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.cc#newcode158 src/x64/lithium-codegen-x64.cc:158: __ push(rsi); // Callee's context. nit: could hoist this out of the if/else blocks. https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.cc#newcode284 src/x64/lithium-codegen-x64.cc:284: bool has_generated_needs_frame_not_call = false; see comments on ia32 version. In short: 1) if you use the Label::is_bound() method, you don't need the separate bool. 2) I'd reorder the blocks to "if (bound) { jump } else { bind }" 3) comments are weird https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.cc#newcode363 src/x64/lithium-codegen-x64.cc:363: Comment(";;; Deferred destory frame", nit: "destroy" https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.h File src/x64/lithium-codegen-x64.h (right): https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x64.h#newcode340 src/x64/lithium-codegen-x64.h:340: explicit inline JumpTableEntry(Address entry, bool frame, bool is_lazy) nit: no need for explicit https://codereview.chromium.org/10701054/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
