Remove spilled code from SetValue and GetValue

Change CallCodeObject to take Results as arguments, 
and to check the type of code object being called.
Remove spilled code surrounding all calls to CallCodeObject.

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

[Kevin Millikin (Chromium), 2009-01-26 17:05:31]

This doesn't look quite right.

The load and store ICs expect values passed on the stack that are not consumed
by the ICs.  We need to ensure that those values are in memory for the IC code.

http://codereview.chromium.org/18596/diff/1/2
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18596/diff/1/2#newcode4607
Line 4607: // They are spilled in CallCodeObject.
As discussed, they are not spilled by CallCodeObject.  The simplest thing is to
SpillAll before the CallCodeObject.

Ultimately we will want something better.

http://codereview.chromium.org/18596/diff/1/2#newcode4611
Line 4611: RelocInfo::CODE_TARGET_CONTEXT, 0);
Put the zero on the next line, lest it get lost.

http://codereview.chromium.org/18596/diff/1/2#newcode4619
Line 4619: // the push that follows might be peep-hole optimized away.
Change the comment about peephole optimization to say something like the push to
the virtual frame will be performed lazily only when needed.

http://codereview.chromium.org/18596/diff/1/2#newcode4621
Line 4621: frame->Push(&answer);  // IC call leaves result in eax, push it out
Just remove this comment.

http://codereview.chromium.org/18596/diff/1/2#newcode4681
Line 4681: Result argument = frame->Pop();
Not really "argument".  It is the value being stored.

http://codereview.chromium.org/18596/diff/1/2#newcode4684
Line 4684: Result ecx_result = cgen_->allocator()->Allocate(ecx);
I think name_register or something is a better name than ecx_result.

http://codereview.chromium.org/18596/diff/1/2#newcode4689
Line 4689: argument.Unuse();
As discussed, we should probably pass these registers to CallCode object to be
consumed, rather than explicitly unusing them.

http://codereview.chromium.org/18596/diff/1/2#newcode4691
Line 4691: Result answer = frame->CallCodeObject(ic, RelocInfo::CODE_TARGET, 0);
I think this needs some sort of spill solution.  The receiver is on the stack,
used by the call, and will not necessarily be spilled.

http://codereview.chromium.org/18596/diff/1/2#newcode4701
Line 4701: Result temp = frame->Pop();
Not really a temp, it is the value being stored.

http://codereview.chromium.org/18596/diff/1/2#newcode4704
Line 4704: temp.Unuse();
Ditto about having CallCodeObject consume the register reference rather than
explicitly unusing it.

http://codereview.chromium.org/18596/diff/1/2#newcode4705
Line 4705: temp = frame->CallCodeObject(ic, RelocInfo::CODE_TARGET, 0);
Same comment about spilling.  The key and receiver are both on the stack and
used by the call, but not necessarily spilled.

[William Hesse, 2009-01-27 15:39:35]

Lots of change to the changelist.  The current bug in this is that all the
inline code functions listed at line 337 of codegen.cc are now called with
non-spilled scopes.

[Kevin Millikin (Chromium), 2009-01-28 08:40:10]

Bill, I'll have time to look at this this afternoon.

For the inlined builtins, you should just track them all down and spill them for
now.

It occurs to me that we have to be careful with fixed registers (like the ecx
used for the name in named property accesses) in SetValue and GetValue.  Usually
when we allocate a specific register it is statically guaranteed to succeed
because all non-reserved registers are available at each Visit method and every
named basic block entry.  Depending on the call site of SetValue and GetValue,
this might not be guaranteed to be the case (I think it currently is---because
they operate on the frame, but we should figure out how to enforce it).  We may
be able to just assert valid entry registers at GetValue and SetValue.

[Kevin Millikin (Chromium), 2009-01-29 09:11:07]

I really like the cleanup of the virtual frame CallXXX functions.  I think it
pays to be more explicit there and at the same time offload some of the setup
from within the code generator.

I'm not sure about the unspilled scope in the middle of object literals.  And I
would like to see the spilled scopes pushed into the actual code generation
functions of the inlined builtins (even if it's only temporary), to increase our
confidence in this change based on testing.

http://codereview.chromium.org/18596/diff/402/601
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18596/diff/402/601#newcode3006
Line 3006: // Beginning of unspilled code, falls through to the function return.
This seems wrong, or at least weird.  Manually setting the spilled code flag is
a bad smell.  All the frame and codegen functions should work if they get a
frame that happens to be spilled, so it seems unnecessary.

It also seems wrong to force ourselves into an unspilled state with a live
spilled scope in the current C++ frame.

And it really seems scary that since this code is in a loop and the other cases
all expect a spilled frame (all the raw accesses to the stack top and raw
pushes), we can be leaving ourselves in an inconsistent state for the remaining
iterations.

http://codereview.chromium.org/18596/diff/402/601#newcode3713
Line 3713: VirtualFrame::SpilledScope spilled_scope(this);
You should remove this spilled scope from here and add it to *all* the inline
runtime calls (the GenerateXXX functions for inline runtime calls, like you did
in GenerateIsArray just above).

Spilling before checking for an inlined call is almost the same as spilling for
every runtime call.  I'm concerned that the following unspilled code isn't
actually getting enough interesting frames from our tests to have confidence in
their coverage.

http://codereview.chromium.org/18596/diff/402/601#newcode3748
Line 3748: &num_args, arg_count + 1);
Indentation seems off.

http://codereview.chromium.org/18596/diff/402/601#newcode4537
Line 4537: void Reference::GetValue(TypeofState typeof_state) {
Since we are allocating explicit registers (eg, eax, ecx) in this snippet, and
that won't work if we have off-frame register references, we should assert that
we have valid entry registers, here and in SetValue.

http://codereview.chromium.org/18596/diff/402/601#newcode4565
Line 4565: __ mov(ecx, name);
__ mov(name_reg.reg(), name);

http://codereview.chromium.org/18596/diff/402/601#newcode4568
Line 4568: frame->CallCodeObject(ic, RelocInfo::CODE_TARGET_CONTEXT, &name_reg,
0);
Since the two alternatives here only differ by their relocation info, it might
be cleaner to set a relocation info variable and then have only a single call to
the code object.  Up to you.

http://codereview.chromium.org/18596/diff/402/601#newcode4572
Line 4572: frame->EmitPush(eax);  // IC call leaves result in eax, push it out
Push the result of the call (rather than discarding it), not  literally eax. 
And remove the comment.

http://codereview.chromium.org/18596/diff/402/601#newcode4669
Line 4669: answer = frame->CallCodeObject(ic,
Same comment about cases that only differ in one subexpression.

http://codereview.chromium.org/18596/diff/402/601#newcode4680
Line 4680: frame->Push(&answer);  // IC call leaves result in eax, push it out
Remove this comment.

http://codereview.chromium.org/18596/diff/402/602
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/18596/diff/402/602#newcode261
Line 261: void VirtualFrame::PrepareForCall(int dropped_args, int spilled_args)
{
Nothing will break in the implementation, but it seems like it might be an error
for a callee to pop more arguments than it was passed.  If so, here might be a
good place to assert that dropped_args <= spilled_args.

http://codereview.chromium.org/18596/diff/402/602#newcode956
Line 956: default:
It's not entirely obvious why the other code kinds cannot reach here (ie, they
are called with register arguments).  This needs a comment at the default case
or before the switch.

http://codereview.chromium.org/18596/diff/402/602#newcode984
Line 984: default:
Ditto.

http://codereview.chromium.org/18596/diff/402/602#newcode1012
Line 1012: default:
Ditto.

http://codereview.chromium.org/18596/diff/402/603
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/18596/diff/402/603#newcode210
Line 210: void SpillTopElements(int number_spilled);
Since this doesn't appear to be called anywhere in this changelist, I would get
rid of it.

If there's a reason to keep it, does it need to be public?  It seems like an
implementaiton utility, not something to expose in the public API.

http://codereview.chromium.org/18596/diff/402/603#newcode369
Line 369: // The optional arguments passed in as Result pointers are Unused()
from
I wouldn't document the (current) implementation so much in the header comments.
 Say something like "Register arguments are passed as results and consumed by
the call."

http://codereview.chromium.org/18596/diff/402/603#newcode538
Line 538: // the frame when the call returns.  Spill all elements in registers.
The comment wording is weird.  It sounds like registers might be spilled after
the call.  Please reword.

http://codereview.chromium.org/18596/diff/402/603#newcode561
Line 561: // Calls a code object, dropping dropped_args arguments from the
"Calls a code object which takes spilled_args frame arguments, of which
dropped_args are popped by the callee."  It seems more natural to me to put
spilled_args before dropped_args in the signature.

[William Hesse, 2009-01-29 10:44:50]

http://codereview.chromium.org/18596/diff/402/601
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18596/diff/402/601#newcode3006
Line 3006: // Beginning of unspilled code, falls through to the function return.
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> This seems wrong, or at least weird.  Manually setting the spilled code flag
is
> a bad smell.  All the frame and codegen functions should work if they get a
> frame that happens to be spilled, so it seems unnecessary.
> 
> It also seems wrong to force ourselves into an unspilled state with a live
> spilled scope in the current C++ frame.
> 
> And it really seems scary that since this code is in a loop and the other
cases
> all expect a spilled frame (all the raw accesses to the stack top and raw
> pushes), we can be leaving ourselves in an inconsistent state for the
remaining
> iterations.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode3713
Line 3713: VirtualFrame::SpilledScope spilled_scope(this);
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> You should remove this spilled scope from here and add it to *all* the inline
> runtime calls (the GenerateXXX functions for inline runtime calls, like you
did
> in GenerateIsArray just above).
> 
> Spilling before checking for an inlined call is almost the same as spilling
for
> every runtime call.  I'm concerned that the following unspilled code isn't
> actually getting enough interesting frames from our tests to have confidence
in
> their coverage.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode3748
Line 3748: &num_args, arg_count + 1);
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Indentation seems off.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4537
Line 4537: void Reference::GetValue(TypeofState typeof_state) {
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Since we are allocating explicit registers (eg, eax, ecx) in this snippet, and
> that won't work if we have off-frame register references, we should assert
that
> we have valid entry registers, here and in SetValue.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4565
Line 4565: __ mov(ecx, name);
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> __ mov(name_reg.reg(), name);

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4568
Line 4568: frame->CallCodeObject(ic, RelocInfo::CODE_TARGET_CONTEXT, &name_reg,
0);
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Since the two alternatives here only differ by their relocation info, it might
> be cleaner to set a relocation info variable and then have only a single call
to
> the code object.  Up to you.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4572
Line 4572: frame->EmitPush(eax);  // IC call leaves result in eax, push it out
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Push the result of the call (rather than discarding it), not  literally eax. 
> And remove the comment.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4669
Line 4669: answer = frame->CallCodeObject(ic,
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Same comment about cases that only differ in one subexpression.

Done.

http://codereview.chromium.org/18596/diff/402/601#newcode4680
Line 4680: frame->Push(&answer);  // IC call leaves result in eax, push it out
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Remove this comment.

Done.

http://codereview.chromium.org/18596/diff/402/602
File src/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/18596/diff/402/602#newcode261
Line 261: void VirtualFrame::PrepareForCall(int dropped_args, int spilled_args)
{
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Nothing will break in the implementation, but it seems like it might be an
error
> for a callee to pop more arguments than it was passed.  If so, here might be a
> good place to assert that dropped_args <= spilled_args.

Done.

http://codereview.chromium.org/18596/diff/402/602#newcode956
Line 956: default:
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> It's not entirely obvious why the other code kinds cannot reach here (ie, they
> are called with register arguments).  This needs a comment at the default case
> or before the switch.

Done.

http://codereview.chromium.org/18596/diff/402/602#newcode984
Line 984: default:
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Ditto.

Done.

http://codereview.chromium.org/18596/diff/402/602#newcode1012
Line 1012: default:
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Ditto.

Done.

http://codereview.chromium.org/18596/diff/402/603
File src/virtual-frame-ia32.h (right):

http://codereview.chromium.org/18596/diff/402/603#newcode210
Line 210: void SpillTopElements(int number_spilled);
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> Since this doesn't appear to be called anywhere in this changelist, I would
get
> rid of it.
> 
> If there's a reason to keep it, does it need to be public?  It seems like an
> implementaiton utility, not something to expose in the public API.

Done.

http://codereview.chromium.org/18596/diff/402/603#newcode369
Line 369: // The optional arguments passed in as Result pointers are Unused()
from
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> I wouldn't document the (current) implementation so much in the header
comments.
>  Say something like "Register arguments are passed as results and consumed by
> the call." 

Done.

http://codereview.chromium.org/18596/diff/402/603#newcode538
Line 538: // the frame when the call returns.  Spill all elements in registers.
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> The comment wording is weird.  It sounds like registers might be spilled after
> the call.  Please reword.

Done.

http://codereview.chromium.org/18596/diff/402/603#newcode561
Line 561: // Calls a code object, dropping dropped_args arguments from the
On 2009/01/29 09:11:07, Kevin Millikin wrote:
> "Calls a code object which takes spilled_args frame arguments, of which
> dropped_args are popped by the callee."  It seems more natural to me to put
> spilled_args before dropped_args in the signature.

Done.

[Kevin Millikin (Chromium), 2009-01-30 09:26:10]

LGTM, except some requested changes to comments.

http://codereview.chromium.org/18596/diff/610/611
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18596/diff/610/611#newcode3734
Line 3734: //  VirtualFrame::SpilledScope spilled_scope(this);
Get rid of this commented code.

http://codereview.chromium.org/18596/diff/610/611#newcode4682
Line 4682: // The arguments to the functions are on top of the frame.
I find this comment more confusing than clarifying.  I'm not sure there needs to
be a comment at all.  Anyway, there are not functions, there is only one; and
it's not exactly a function but an IC stub.

http://codereview.chromium.org/18596/diff/610/611#newcode4762
Line 4762: // Free the registers used by the call.
This comment doesn't entirely make sense.

[William Hesse, 2009-01-30 11:42:23]

http://codereview.chromium.org/18596/diff/610/611
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/18596/diff/610/611#newcode3734
Line 3734: //  VirtualFrame::SpilledScope spilled_scope(this);
On 2009/01/30 09:26:10, Kevin Millikin wrote:
> Get rid of this commented code.

Done.

http://codereview.chromium.org/18596/diff/610/611#newcode4682
Line 4682: // The arguments to the functions are on top of the frame.
On 2009/01/30 09:26:10, Kevin Millikin wrote:
> I find this comment more confusing than clarifying.  I'm not sure there needs
to
> be a comment at all.  Anyway, there are not functions, there is only one; and
> it's not exactly a function but an IC stub.

Done.

http://codereview.chromium.org/18596/diff/610/611#newcode4762
Line 4762: // Free the registers used by the call.
On 2009/01/30 09:26:10, Kevin Millikin wrote:
> This comment doesn't entirely make sense.

Done.