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#newcode264
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#newcode41
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#newcode3808
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#newcode260
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-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",
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-arm.cc#newcode312
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-arm.cc#newcode328
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-arm.cc#newcode1451
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-arm.cc#newcode1738
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-arm.cc#newcode1908
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-arm.cc#newcode1955
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-arm.cc#newcode2047
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-arm.cc#newcode2214
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-arm.cc#newcode3700
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-arm.cc#newcode3738
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-arm.cc#newcode3765
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-arm.cc#newcode3832
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-arm.cc#newcode3841
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-arm.cc#newcode3864
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-arm.cc#newcode3898
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-arm.cc#newcode3978
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-arm.cc#newcode4269
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-arm.cc#newcode4341
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-arm.cc#newcode4619
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-arm.cc#newcode4638
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-arm.cc#newcode4726
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-arm.cc#newcode4904
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-arm.cc#newcode4980
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-arm.cc#newcode5222
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-arm.cc#newcode5232
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-arm.cc#newcode5241
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-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)
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-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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
On 2012/11/28 16:28:22, Jakob wrote:
nit: not needed
Done.
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));
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.cc#newcode53
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.cc#newcode57
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.cc#newcode61
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.cc#newcode80
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.cc#newcode112
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#newcode169
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#newcode185
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.cc#newcode45
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.cc#newcode47
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.cc#newcode595
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-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);
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-ia32.cc#newcode199
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-ia32.cc#newcode364
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-ia32.cc#newcode368
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-ia32.cc#newcode389
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-ia32.cc#newcode393
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-ia32.cc#newcode443
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-ia32.cc#newcode1604
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-ia32.cc#newcode1821
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-ia32.cc#newcode3833
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-ia32.cc#newcode5079
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-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)
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-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.
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#newcode823
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#newcode1431
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-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)
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#newcode1331
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#newcode1
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#newcode66
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#newcode205
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#newcode654
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#newcode674
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#newcode677
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#newcode41
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-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.
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-x64.cc#newcode284
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-x64.cc#newcode363
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-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)
On 2012/11/28 16:28:22, Jakob wrote:
nit: no need for explicit
Done.
https://codereview.chromium.org/10701054/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
