Enable stub generation using Hydrogen/Lithium (again)

Enable stub generation using Hydrogen/Lithium (again)

This initial implementation generates only KeyedLoadICs using the new Hydrogen stub infrastructure.

Committed: https://code.google.com/p/v8/source/detail?r=13105

Committed: https://code.google.com/p/v8/source/detail?r=13117

Committed: https://code.google.com/p/v8/source/detail?r=13140

[Jakob Kummerow, 2012-11-19 12:36:00]

First round of comments, just to keep you busy :-)

https://codereview.chromium.org/10701054/diff/69002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3792:
save_doubles.GetCode()->set_is_pregenerated(true);
Suggestion: save the result of save_doubles.GetCode in a variable, and do the
->set_is_pregenerated(true) call after the if/else-block.

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:464: void
Deoptimizer::DoCompiledStubPseudoFrame(TranslationIterator* iterator,
s/Pseudo//

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:467: //  Code::Kind stub_kind =
static_cast<Code::Kind>(iterator->Next());
remove

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:468: FrameDescription* output_frame = new(0)
FrameDescription(0, 0);
just "NULL"?

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:474: Handle<Code> miss_ic =
isolate_->builtins()->KeyedLoadIC_Miss();
use stub_kind (see line 478) to figure this out

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2146: case KEYED_STORE_IC_PARAMETER:
remove for now

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:592:
translation->BeginCompiledStubPseudoFrame(Code::KEYED_LOAD_IC);
can we get the code type dynamically?

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:811: __ Jump(entry, RelocInfo::RUNTIME_ENTRY);
nit: indentation

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4597:
FloatingPointHelper::ConvertIntToDouble(masm(),
nit: fix format

https://codereview.chromium.org/10701054/diff/69002/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:2700: CEntryStub stub(1,
CpuFeatures::IsSupported(VFP2)
nit: formatting

https://codereview.chromium.org/10701054/diff/69002/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/code-stubs.cc#newcode71
src/code-stubs.cc:71: stub->Print();
Don't forget to remove this (or surround with "if (FLAG_print_code)").

https://codereview.chromium.org/10701054/diff/69002/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/disassembler.cc#newcode1
src/disassembler.cc:1: // Copyright 2012 the V8 project authors. All rights
reserved.
we don't do this kind of change any more.

https://codereview.chromium.org/10701054/diff/69002/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/flag-definitions.h#ne...
src/flag-definitions.h:286: DEFINE_bool(enable_vfp3, false,
don't forget to undo this change

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:382: HValue* operand = OperandAt(i);
as discussed, just pass in any existing HValue as fake dependency, so you don't
need this code.
If you do keep it, replace OperandAt(i) below with |operand| ;-)

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions...
src/hydrogen-instructions.h:3775: KEYED_STORE_IC_PARAMETER
Remove this

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode32
src/hydrogen.cc:32: #include "elements-hydrogen.h"
remove

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode280
src/hydrogen.cc:280: HEnvironment* last = pred->last_environment();
remove this change?

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode6096
src/hydrogen.cc:6096: checked_key,
nit: indentation

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode9829
src/hydrogen.cc:9829: PrintStringProperty("name", "stub");
do we have more information here maybe? stub type?

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.h#newcode1125
src/hydrogen.h:1125: HLoadNamedField* BuildLoadNamedField(HValue* object,
merge problem?

https://codereview.chromium.org/10701054/diff/69002/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/ia32/assembler-ia32.h...
src/ia32/assembler-ia32.h:219: struct X87TopOfStackProxyRegister :
IntelDoubleRegister {
s/Proxy//

https://codereview.chromium.org/10701054/diff/69002/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/log.cc#newcode1
src/log.cc:1: // Copyright 2012 the V8 project authors. All rights reserved.
you know..

https://codereview.chromium.org/10701054/diff/69002/src/safepoint-table.cc
File src/safepoint-table.cc (left):

https://codereview.chromium.org/10701054/diff/69002/src/safepoint-table.cc#ol...
src/safepoint-table.cc:164: int target_offset = assembler->pc_offset() +
Deoptimizer::patch_size();
Please make sure it's safe not to do this.

https://codereview.chromium.org/10701054/diff/69002/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/serialize.cc#newcode540
src/serialize.cc:540: Add(address, LAZY_DEOPTIMIZATION, 51 + entry,
"lazy_deopt");
52!

https://codereview.chromium.org/10701054/diff/69002/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/utils.h#newcode1035
src/utils.h:1035: static const int kFirstUsableId = 4;
every

https://codereview.chromium.org/10701054/diff/69002/src/utils.h#newcode1038
src/utils.h:1038: static const int kStubEntryId = 5;
every

https://codereview.chromium.org/10701054/diff/69002/test/mjsunit/keyed-call-i...
File test/mjsunit/keyed-call-ic.js (left):

https://codereview.chromium.org/10701054/diff/69002/test/mjsunit/keyed-call-i...
test/mjsunit/keyed-call-ic.js:64: var f = new F();
uhm...?

https://codereview.chromium.org/10701054/diff/69002/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/10701054/diff/69002/tools/gyp/v8.gyp#newcode350
tools/gyp/v8.gyp:350: '../../src/hydrogen.cc',
might wanna remove this.

[danno, 2012-11-26 17:16:17]

please take another look :-)

https://codereview.chromium.org/10701054/diff/69002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3792:
save_doubles.GetCode()->set_is_pregenerated(true);
On 2012/11/19 12:36:00, Jakob wrote:
> Suggestion: save the result of save_doubles.GetCode in a variable, and do the
> ->set_is_pregenerated(true) call after the if/else-block.

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.cc
File src/arm/deoptimizer-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:464: void
Deoptimizer::DoCompiledStubPseudoFrame(TranslationIterator* iterator,
On 2012/11/19 12:36:00, Jakob wrote:
> s/Pseudo//

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:467: //  Code::Kind stub_kind =
static_cast<Code::Kind>(iterator->Next());
On 2012/11/19 12:36:00, Jakob wrote:
> remove

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:468: FrameDescription* output_frame = new(0)
FrameDescription(0, 0);
On 2012/11/19 12:36:00, Jakob wrote:
> just "NULL"?

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/deoptimizer-arm.c...
src/arm/deoptimizer-arm.cc:474: Handle<Code> miss_ic =
isolate_->builtins()->KeyedLoadIC_Miss();
On 2012/11/19 12:36:00, Jakob wrote:
> use stub_kind (see line 478) to figure this out

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-arm.cc#ne...
src/arm/lithium-arm.cc:2146: case KEYED_STORE_IC_PARAMETER:
On 2012/11/19 12:36:00, Jakob wrote:
> remove for now

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:592:
translation->BeginCompiledStubPseudoFrame(Code::KEYED_LOAD_IC);
On 2012/11/19 12:36:00, Jakob wrote:
> can we get the code type dynamically?

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:811: __ Jump(entry, RelocInfo::RUNTIME_ENTRY);
On 2012/11/19 12:36:00, Jakob wrote:
> nit: indentation

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4597:
FloatingPointHelper::ConvertIntToDouble(masm(),
On 2012/11/19 12:36:00, Jakob wrote:
> nit: fix format

Done.

https://codereview.chromium.org/10701054/diff/69002/src/arm/macro-assembler-a...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/arm/macro-assembler-a...
src/arm/macro-assembler-arm.cc:2700: CEntryStub stub(1,
CpuFeatures::IsSupported(VFP2)
On 2012/11/19 12:36:00, Jakob wrote:
> nit: formatting

Done.

https://codereview.chromium.org/10701054/diff/69002/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/code-stubs.cc#newcode71
src/code-stubs.cc:71: stub->Print();
On 2012/11/19 12:36:00, Jakob wrote:
> Don't forget to remove this (or surround with "if (FLAG_print_code)").

Done.

https://codereview.chromium.org/10701054/diff/69002/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/disassembler.cc#newcode1
src/disassembler.cc:1: // Copyright 2012 the V8 project authors. All rights
reserved.
On 2012/11/19 12:36:00, Jakob wrote:
> we don't do this kind of change any more.

Done.

https://codereview.chromium.org/10701054/diff/69002/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/flag-definitions.h#ne...
src/flag-definitions.h:286: DEFINE_bool(enable_vfp3, false,
On 2012/11/19 12:36:00, Jakob wrote:
> don't forget to undo this change

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:382: HValue* operand = OperandAt(i);
On 2012/11/19 12:36:00, Jakob wrote:
> as discussed, just pass in any existing HValue as fake dependency, so you
don't
> need this code.
> If you do keep it, replace OperandAt(i) below with |operand| ;-)

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen-instructions...
src/hydrogen-instructions.h:3775: KEYED_STORE_IC_PARAMETER
On 2012/11/19 12:36:00, Jakob wrote:
> Remove this

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode32
src/hydrogen.cc:32: #include "elements-hydrogen.h"
On 2012/11/19 12:36:00, Jakob wrote:
> remove

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode280
src/hydrogen.cc:280: HEnvironment* last = pred->last_environment();
On 2012/11/19 12:36:00, Jakob wrote:
> remove this change?

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode6096
src/hydrogen.cc:6096: checked_key,
On 2012/11/19 12:36:00, Jakob wrote:
> nit: indentation

Done.

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.cc#newcode9829
src/hydrogen.cc:9829: PrintStringProperty("name", "stub");
unfortunately, no.

On 2012/11/19 12:36:00, Jakob wrote:
> do we have more information here maybe? stub type?

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/hydrogen.h#newcode1125
src/hydrogen.h:1125: HLoadNamedField* BuildLoadNamedField(HValue* object,
On 2012/11/19 12:36:00, Jakob wrote:
> merge problem?

Done.

https://codereview.chromium.org/10701054/diff/69002/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/ia32/assembler-ia32.h...
src/ia32/assembler-ia32.h:219: struct X87TopOfStackProxyRegister :
IntelDoubleRegister {
On 2012/11/19 12:36:00, Jakob wrote:
> s/Proxy//

Done.

https://codereview.chromium.org/10701054/diff/69002/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/log.cc#newcode1
src/log.cc:1: // Copyright 2012 the V8 project authors. All rights reserved.
On 2012/11/19 12:36:00, Jakob wrote:
> you know..

Done.

https://codereview.chromium.org/10701054/diff/69002/src/safepoint-table.cc
File src/safepoint-table.cc (left):

https://codereview.chromium.org/10701054/diff/69002/src/safepoint-table.cc#ol...
src/safepoint-table.cc:164: int target_offset = assembler->pc_offset() +
Deoptimizer::patch_size();
On 2012/11/19 12:36:00, Jakob wrote:
> Please make sure it's safe not to do this.

Done.

https://codereview.chromium.org/10701054/diff/69002/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/10701054/diff/69002/src/serialize.cc#newcode540
src/serialize.cc:540: Add(address, LAZY_DEOPTIMIZATION, 51 + entry,
"lazy_deopt");
On 2012/11/19 12:36:00, Jakob wrote:
> 52!

Done.

https://codereview.chromium.org/10701054/diff/69002/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/10701054/diff/69002/src/utils.h#newcode1035
src/utils.h:1035: static const int kFirstUsableId = 4;
On 2012/11/19 12:36:00, Jakob wrote:
> every

Done.

https://codereview.chromium.org/10701054/diff/69002/src/utils.h#newcode1038
src/utils.h:1038: static const int kStubEntryId = 5;
On 2012/11/19 12:36:00, Jakob wrote:
> every

Done.

https://codereview.chromium.org/10701054/diff/69002/test/mjsunit/keyed-call-i...
File test/mjsunit/keyed-call-ic.js (left):

https://codereview.chromium.org/10701054/diff/69002/test/mjsunit/keyed-call-i...
test/mjsunit/keyed-call-ic.js:64: var f = new F();
On 2012/11/19 12:36:00, Jakob wrote:
> uhm...?

Done.

https://codereview.chromium.org/10701054/diff/69002/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/10701054/diff/69002/tools/gyp/v8.gyp#newcode350
tools/gyp/v8.gyp:350: '../../src/hydrogen.cc',
On 2012/11/19 12:36:00, Jakob wrote:
> might wanna remove this.

Done.

[Jakob Kummerow, 2012-11-28 16:28:22]

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#n...
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...
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...
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#new...
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-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:291: Comment(";;; Deferred destory frame",
nit: "destroy"

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
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-a...
File src/arm/lithium-codegen-arm.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
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-resol...
File src/arm/lithium-gap-resolver-arm.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
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-resol...
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-resol...
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-resol...
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-resol...
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-resol...
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-resol...
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.c...
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.c...
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.c...
src/code-stubs-hydrogen.cc:61: private:
nit: empty line before visibility sections

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:80: CodeStubInterfaceDescriptor** decsriptors =
nit: descriptors

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
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...
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...
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....
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....
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...
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-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
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-...
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-...
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-...
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-...
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-...
src/ia32/lithium-codegen-ia32.cc:443: Comment(";;; Deferred destory frame",
nit: "destroy"

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
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-...
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-...
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-...
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-...
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-...
File src/ia32/lithium-codegen-ia32.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
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-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-gap-reso...
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#...
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#n...
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-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/macro-assembler-...
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#...
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#ne...
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#newc...
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#...
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#n...
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#n...
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#n...
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...
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-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
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-x...
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-x...
src/x64/lithium-codegen-x64.cc:363: Comment(";;; Deferred destory frame",
nit: "destroy"

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.h:340: explicit inline JumpTableEntry(Address entry,
bool frame, bool is_lazy)
nit: no need for explicit

[danno, 2012-11-30 16:23:24]

Feedback addressed, landing.

https://codereview.chromium.org/10701054/diff/67140/Makefile
File Makefile (right):

https://codereview.chromium.org/10701054/diff/67140/Makefile#newcode84
Makefile:84: # vfp2=on
On 2012/11/28 16:28:22, Jakob wrote:
> nit: the comment is supposed to indicate the explicit flag being handled here,
> i.e. the non-default value (see examples above and below).

Done.

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#n...
src/arm/assembler-arm.h:264: DwVfpRegister r = DwVfpRegister(code);
On 2012/11/28 16:28:22, Jakob wrote:
> nit: how about just:
> return DwVfpRegister(code);

Done.

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...
src/arm/code-stubs-arm.cc:41:
KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) {
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation please

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:3808: // regenerate, which leads to code stub
initialization state being messed up.
On 2012/11/28 16:28:22, Jakob wrote:
> s/leads/would lead/?

Done.

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#new...
src/arm/lithium-arm.h:260: bool ClobbersDoubleRegisters() const { return
is_call_; }
ia32 needs this because the register allocator must immediately flush the fp
stack after every instruction if it's not immediately used by the next
instruction, because if the value is unused, following instructions could
generate more floating point values, pushing even more of the fp stack, which
would become unbalanced. So it's important to deal with the top-of-stack
immediately. This is not the case on ARM, where the FP register is backed by
real registers that you can overwrite as many times as you want without
side-effect, so they can be treated just like all other registers. 

On 2012/11/28 16:28:22, Jakob wrote:
> Why doesn't this need " || !CpuFeatures::IsSupported(VFP2);" as ia32 does?

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:291: Comment(";;; Deferred destory frame",
Done. But destroy is so final, so... depressing. "die story" (granted, the
missing "i" and space are indeed nits) was meant to be more uplifting... giving
the reader hope, perhaps there is a life after death? I would have preferred to
not quash that optimism, all other things being equal.

On 2012/11/28 16:28:22, Jakob wrote:
> nit: "destroy"

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:312: // Check that the jump table is acvcessible
from everywhere in the function
On 2012/11/28 16:28:22, Jakob wrote:
> nit: I prefer the old version.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:328: bool has_generated_needs_frame_not_call =
false;
On 2012/11/28 16:28:22, Jakob wrote:
> 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

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:1451: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed, as the Scope c'tor right below contains the same ASSERT.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:1738: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:1908: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:1955: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:2047: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:2214: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3700: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3738: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3765: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3832: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3841: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3864: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3898: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:3978: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4269: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4341: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4619: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4638: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4726: Operand(hiword, LSL,
mantissa_shift_for_hi_word));
On 2012/11/28 16:28:22, Jakob wrote:
> 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 :-)

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4904: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4980: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5222: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5232: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5241: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.h:382: explicit inline JumpTableEntry(Address entry,
bool frame, bool is_lazy)
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
File src/arm/lithium-gap-resolver-arm.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:174: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:178: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:199: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:203: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:240: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:280: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/arm/lithium-gap-resol...
src/arm/lithium-gap-resolver-arm.cc:291: ASSERT(CpuFeatures::IsSupported(VFP2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

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:                                                         
     \
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ast.h#newcode2531
src/ast.h:2531: private:                                                        
       \
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation

Done.

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.c...
src/code-stubs-hydrogen.cc:53: explicit CodeStubGraphBuilderBase(Isolate*
isolate,
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:57: protected:
On 2012/11/28 16:28:22, Jakob wrote:
> nit: empty line before visibility sections (doesn't presubmit.py complain
about
> this?)

Nope. Runs clean.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:61: private:
On 2012/11/28 16:28:22, Jakob wrote:
> nit: empty line before visibility sections

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:80: CodeStubInterfaceDescriptor** decsriptors =
On 2012/11/28 16:28:22, Jakob wrote:
> nit: descriptors

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:112: private:
On 2012/11/28 16:28:22, Jakob wrote:
> nit: don't need an empty "private:" section

Done.

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());
On 2012/11/28 16:28:22, Jakob wrote:
> nit: fits on previous line?

Done.

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;
On 2012/11/28 16:28:22, Jakob wrote:
> 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.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode247
src/code-stubs.h:247: struct CodeStubInterfaceDescriptor {
I like it too!
On 2012/11/28 16:28:22, Jakob wrote:
> Nice!

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1058
src/code-stubs.h:1058: explicit KeyedLoadDictionaryElementStub() {}
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1072
src/code-stubs.h:1072: explicit KeyedLoadFastElementStub(bool is_js_array,
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/code-stubs.h#newcode1074
src/code-stubs.h:1074: bit_field_ = ElementsKindBits::encode(elements_kind);
On 2012/11/28 16:28:22, Jakob wrote:
> nit: elsewhere, we use the pattern:
> bit_field_ = Foo::encode(foo) | Bar::encode(bar);

Done.

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("");
On 2012/11/28 16:28:22, Jakob wrote:
> use info->stub()->Major()?

Done.

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: 
On 2012/11/28 16:28:22, Jakob wrote:
> nit: why remove the second empty line?

Done.

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();
Agreed. It's ugly.
On 2012/11/28 16:28:22, Jakob wrote:
> 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: {
On 2012/11/28 16:28:22, Jakob wrote:
> why this block? Shouldn't make a difference if you just remove the braces.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode3326
src/hydrogen.cc:3326: Expression* expr,
On 2012/11/28 16:28:22, Jakob wrote:
> nit: line break before first argument not necessary?

Done.

https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode4895
src/hydrogen.cc:4895: HOptimizedGraphBuilder::LookupGlobalProperty(
On 2012/11/28 16:28:22, Jakob wrote:
> nit: I think I'd indent this line (by 4 spaces; and then the next by 4 more).

Done.

https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.cc#newcode9818
src/hydrogen.cc:9818: PrintStringProperty("name", "stub");
On 2012/11/28 16:28:22, Jakob wrote:
> Can we print info->stub()->major_key() or something?

Done.

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,
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/hydrogen.h#newcode1304
src/hydrogen.h:1304: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
On 2012/11/28 16:28:22, Jakob wrote:
> These used to be public; did you move them to the private section
intentionally?

Done.

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...
src/ia32/assembler-ia32.h:169: bool is(IntelDoubleRegister reg) const { return
code_ == reg.code_; }
On 2012/11/28 16:28:22, Jakob wrote:
> Wouldn't this return xmm0.is(x87tos) == true? Is that intentional?

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/assembler-ia32.h...
src/ia32/assembler-ia32.h:185: XMMRegister r = XMMRegister(code);
On 2012/11/28 16:28:22, Jakob wrote:
> just "return XMMRegister(code);"?

Done.

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....
src/ia32/code-stubs-ia32.cc:45:
KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) {
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation please

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:47: static CodeStubInterfaceDescriptor* result;
On 2012/11/28 16:28:22, Jakob wrote:
> 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.)

Done.

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...
src/ia32/deoptimizer-ia32.cc:595: Code* continuation =
On 2012/11/28 16:28:22, Jakob wrote:
> Please choose another name either for this or for the "JSFunction
continuation"
> in the diagram above. (Same on the other platforms.)

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:199: __ push(esi);
On 2012/11/28 16:28:22, Jakob wrote:
> nit: could hoist this out of the if/else blocks.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:199: __ push(esi);
On 2012/11/28 16:28:22, Jakob wrote:
> nit: could hoist this out of the if/else blocks.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:364: if (!has_generated_needs_frame_is_call) {
On 2012/11/28 16:28:22, Jakob wrote:
> 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);
>   // ...
> }

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:368: // If there is not frame, we don't have
access to the JSFunction that
On 2012/11/28 16:28:22, Jakob wrote:
> "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, ..."?

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:389: if (!has_generated_needs_frame_not_call) {
On 2012/11/28 16:28:22, Jakob wrote:
> same here

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:393: // If there is not frame, we don't have
access to the JSFunction that
On 2012/11/28 16:28:22, Jakob wrote:
> see above.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:443: Comment(";;; Deferred destory frame",
On 2012/11/28 16:28:22, Jakob wrote:
> nit: "destroy"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:1604: ASSERT(CpuFeatures::IsSupported(SSE2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed. The Scope c'tor right below contains the same ASSERT.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:1821: ASSERT(CpuFeatures::IsSupported(SSE2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:3833: ASSERT(CpuFeatures::IsSupported(SSE2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5079: ASSERT(CpuFeatures::IsSupported(SSE2));
On 2012/11/28 16:28:22, Jakob wrote:
> nit: not needed

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.h:373: explicit inline JumpTableEntry(Address
entry, bool frame, bool is_lazy)
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for "explicit"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-gap-reso...
File src/ia32/lithium-gap-resolver-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/lithium-gap-reso...
src/ia32/lithium-gap-resolver-ia32.cc:1: // Copyright 2012 the V8 project
authors. All rights reserved.
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for this change

Done.

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#...
src/ia32/lithium-ia32.cc:823: if (graph()->info()->IsOptimizing()) {
It looks like subsequent changes obviated the need for this, I removed it on
ia32. 
On 2012/11/28 16:28:22, Jakob wrote:
> 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#n...
src/ia32/lithium-ia32.h:1431: return
!IsDoubleOrFloatElementsKind(hydrogen()->elements_kind());
On 2012/11/28 16:28:22, Jakob wrote:
> Why the "!"? Doesn't it clobber the x87tos register precisely when the
> elements_kind *is* a floating-point kind?

Everything but double types clobber, but a double load redefines the
top-of-stack, and that can't happen simultaneously in an instruction that
clobbers doubles (Register Allocator weirdness), so this is correct.

https://codereview.chromium.org/10701054/diff/67140/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:1804: CEntryStub ces(1,
CpuFeatures::IsSupported(SSE2)
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation / line break.
>   CEntryStub ces(1, CpuFeatures::IsSupported(SSE2) ? kSaveFPRegs
>                                                    : kDontSaveFPRegs);

Done.

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();
On 2012/11/28 16:28:22, Jakob wrote:
> nit: fits on one line

Done.

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#...
src/lithium-allocator.cc:1331: }
On 2012/11/28 16:28:22, Jakob wrote:
> Again, it might be mighty helpful to print
*chunk_->info()->stub()->major_key()
> in the "else" case.

Done.

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.
On 2012/11/28 16:28:22, Jakob wrote:
> nit: unnecessary change. Oh well.

Done.

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#ne...
src/safepoint-table.cc:1: // Copyright 2012 the V8 project authors. All rights
reserved.
On 2012/11/28 16:28:22, Jakob wrote:
> nit: unnecessary change

Done.

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#newc...
src/smart-pointers.h:66: // You can use [n] to index as if it was a plain
pointer
On 2012/11/28 16:28:22, Jakob wrote:
> nit: trailing full stop please.

Done.

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.
On 2012/11/28 16:28:22, Jakob wrote:
> nit: unnecessary change

Done.

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#...
src/x64/assembler-x64.cc:205:
Register::kRegisterCodeByAllocationIndex[kMaxNumAllocatableRegisters] = {
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation please

Done.

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#n...
src/x64/builtins-x64.cc:654: // Preserve registers across notification this is
important compiled stubs
On 2012/11/28 16:28:22, Jakob wrote:
> nit: missing comma after "notification", important *for* compiled stubs

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc#n...
src/x64/builtins-x64.cc:674: // Preserve registers across notification, this is
important compiled stubs
On 2012/11/28 16:28:22, Jakob wrote:
> nit: important *for* compiled stubs

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/builtins-x64.cc#n...
src/x64/builtins-x64.cc:677: __ Pushad();
This is a cut/paste problem. Removed.
On 2012/11/28 16:28:22, Jakob wrote:
> 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...
src/x64/code-stubs-x64.cc:41:
KeyedLoadFastElementStub::GetInterfaceDescriptor(Isolate* isolate) {
On 2012/11/28 16:28:22, Jakob wrote:
> nit: indentation please

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:158: __ push(rsi);  // Callee's context.
On 2012/11/28 16:28:22, Jakob wrote:
> nit: could hoist this out of the if/else blocks.

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:284: bool has_generated_needs_frame_not_call =
false;
On 2012/11/28 16:28:22, Jakob wrote:
> 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

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:363: Comment(";;; Deferred destory frame",
On 2012/11/28 16:28:22, Jakob wrote:
> nit: "destroy"

Done.

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.h (right):

https://codereview.chromium.org/10701054/diff/67140/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.h:340: explicit inline JumpTableEntry(Address entry,
bool frame, bool is_lazy)
On 2012/11/28 16:28:22, Jakob wrote:
> nit: no need for explicit

Done.

[Jakob Kummerow, 2012-11-30 16:43:01]

Still LGTM.

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#newcode1304
src/hydrogen.h:1304: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS();
On 2012/11/30 16:23:24, danno wrote:
> On 2012/11/28 16:28:22, Jakob wrote:
> > These used to be public; did you move them to the private section
> intentionally?
> 
> Done.

Actually, since the macro's expansions contains appropriate public/private
section headers, it doesn't matter at all. Never mind.

https://codereview.chromium.org/10701054/diff/76003/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/10701054/diff/76003/src/ast.h#newcode2515
src/ast.h:2515: public:                                                         
   \
I meant: 1 space indentation for "public:"

https://codereview.chromium.org/10701054/diff/76003/src/ast.h#newcode2531
src/ast.h:2531: private:                                                        
   \
I meant: 1 space indentation for "private:"

https://codereview.chromium.org/10701054/diff/76003/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/10701054/diff/76003/src/ia32/assembler-ia32.h...
src/ia32/assembler-ia32.h:183: XMMRegister r = XMMRegister(code);
It seems you forgot to change this. I suggested to simply "return
XMMRegister(code);".

[danno, 2012-12-03 14:54:32]

Please take a look (patch set 25)

[Jakob Kummerow, 2012-12-03 15:01:05]

patch set 26 LGTM.