Add dynamic stack frame alignment to optimized functions with untagged doubles on the stack.

Add dynamic stack frame alignment to optimized functions with untagged doubles on the stack.

BUG=
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=9415

[Kevin Millikin (Chromium), 2011-09-23 09:38:49]

I like this approach!  It's very simple.

I've got some comments below.

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),
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.

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...
src/ia32/deoptimizer-ia32.cc:444: int frame_pointer =
output_[0]->GetRegister(ebp.code());
Don't put this here (i.e., don't set ebp register and then patch it up just
later).  It's too easy to miss what's going on.

Please just set it to the value you want in the first place, about 8 lines
above.

http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc...
src/ia32/deoptimizer-ia32.cc:502: // If the optimized frame had alignment
padding, adjust the frame pointer
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.

http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc...
src/ia32/deoptimizer-ia32.cc:514: input_->GetRegister(ebp.code()) - (2 *
kPointerSize) - height_in_bytes;
Here:

input_->GetRegister(ebp.code()) - (2 * kPointerSize) - height_in_bytes +
(has_alignment_padding_ * kPointerSize)

http://codereview.chromium.org/7976024/diff/5001/src/ia32/deoptimizer-ia32.cc...
src/ia32/deoptimizer-ia32.cc:565: ASSERT(!is_bottommost ||
input_->GetRegister(ebp.code()) == fp_value);
Here:

ASSERT(!is_bottommost ||
       input_->GetRegister(ebp.code()) + has_alignment_padding * kPointerSize ==
fp_value);

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:160: __ mov(ebx, esp);
Can you test(esp, 4)?

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:161: __ and_(ebx, Immediate(0x4));
Is this better (kDoubleSize - 1) or (kDoubleSize >> 1) or some other named
constant?

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:163: __ mov(ebx, esp);
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.

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:165: __ mov(ecx,
Immediate(scope()->num_parameters() + 2));
Comment should say 2 is receiver + return address.

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:2045: __ cmp(Operand(ebp, (GetParameterCount()
+ 3) * kPointerSize),
3 ~ caller's fp, return address, receiver.

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:2049: __ pop(ebp);
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);

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-ia3...
src/ia32/lithium-codegen-ia32.h:138: void set_dynamic_frame_alignment(bool
value) {
I don't think you need a private setter that has one call site.

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;
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.

[William Hesse, 2011-09-23 12:50:10]

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...
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...
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...
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-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7976024/diff/5001/src/ia32/lithium-codegen-ia3...
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-ia3...
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-ia3...
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-ia3...
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-ia3...
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-ia3...
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-ia3...
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.

[Kevin Millikin (Chromium), 2011-09-23 13:20:35]

LGTM!