http://codereview.chromium.org/7976024/diff/5001/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/7976024/diff/5001/src/heap.cc#newcode2246
src/heap.cc:2246: Smi::FromInt(-5),
On 2011/09/23 09:38:49, Kevin Millikin wrote:
I guess it's useful to give the hidden oddballs distinct toNumber
values, but
then you should either reorder their allocation so the numbers are
consecutive
or renumber them.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):
http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode502
src/ia32/deoptimizer-ia32.cc:502: // If the optimized frame had
alignment padding, adjust the frame pointer
On 2011/09/23 09:38:49, Kevin Millikin wrote:
I also don't like this here. First, it breaks up the comment about
setting the
top address so it's now far from the code that sets the top address.
Worse, I think it's a bug waiting to happen to just spoof the value in
the input
ebp register. I'd rather leave the correct value there and adjust for
padding
in a couple of places.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode514
src/ia32/deoptimizer-ia32.cc:514: input_->GetRegister(ebp.code()) - (2 *
kPointerSize) - height_in_bytes;
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Here:
input_->GetRegister(ebp.code()) - (2 * kPointerSize) - height_in_bytes
+
(has_alignment_padding_ * kPointerSize)
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc#newcode565
src/ia32/deoptimizer-ia32.cc:565: ASSERT(!is_bottommost ||
input_->GetRegister(ebp.code()) == fp_value);
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Here:
ASSERT(!is_bottommost ||
input_->GetRegister(ebp.code()) + has_alignment_padding *
kPointerSize ==
fp_value);
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode160
src/ia32/lithium-codegen-ia32.cc:160: __ mov(ebx, esp);
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Can you test(esp, 4)?
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode161
src/ia32/lithium-codegen-ia32.cc:161: __ and_(ebx, Immediate(0x4));
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Is this better (kDoubleSize - 1) or (kDoubleSize >> 1) or some other
named
constant?
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode163
src/ia32/lithium-codegen-ia32.cc:163: __ mov(ebx, esp);
On 2011/09/23 09:38:49, Kevin Millikin wrote:
The last move after the loop might be slightly more obvious if you
make ebx be
the address you're copying to, rather than from.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode165
src/ia32/lithium-codegen-ia32.cc:165: __ mov(ecx,
Immediate(scope()->num_parameters() + 2));
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Comment should say 2 is receiver + return address.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode2045
src/ia32/lithium-codegen-ia32.cc:2045: __ cmp(Operand(ebp,
(GetParameterCount() + 3) * kPointerSize),
On 2011/09/23 09:38:49, Kevin Millikin wrote:
3 ~ caller's fp, return address, receiver.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.cc#newcode2049
src/ia32/lithium-codegen-ia32.cc:2049: __ pop(ebp);
On 2011/09/23 09:38:49, Kevin Millikin wrote:
Slightly more compact is:
__ mov(esp, ebp);
__ pop(ebp);
__ cmp(Operand(esp, (param_count + 2) * kPointerSize), marker);
__ j(not_equal, &aligned);
__ Ret(parameter_count + 2);
__ bind(&aligned);
__ Ret(parameter_count + 1);
Done.
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):
http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia32.h#newcode138
src/ia32/lithium-codegen-ia32.h:138: void
set_dynamic_frame_alignment(bool value) {
On 2011/09/23 09:38:49, Kevin Millikin wrote:
I don't think you need a private setter that has one call site.
Done.
http://codereview.chromium.org/7976024/diff/5001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7976024/diff/5001/src/objects.h#newcode6581
src/objects.h:6581: static const byte kFrameAlignmentMarker = 6;
On 2011/09/23 09:38:49, Kevin Millikin wrote:
You never use this constant that I can see. Can you just reuse
kOther? I'm not
sure it's critical to have as many tags as oddballs.
Done.
http://codereview.chromium.org/7976024/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
