Simplify compilation of CallIC interceptor stub.

Simplify compilation of CallIC interceptor stub.

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

[antonm, 2010-02-15 08:47:37]

LGTM

http://codereview.chromium.org/604054/diff/1/2
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/604054/diff/1/2#newcode1055
src/arm/stub-cache-arm.cc:1055: <<<<<<< .mine
merge conflict

http://codereview.chromium.org/604054/diff/1/2#newcode1065
src/arm/stub-cache-arm.cc:1065: CallInterceptorCompiler compiler(arguments());
1) probably the whole compiler should go away as it now only have one method;
2) I am slightly concerned that various platform now diverge---could we bring
the similar change to other platforms?

[William Hesse, 2010-02-15 10:32:55]

Cleanup ready to commit.

http://codereview.chromium.org/604054/diff/1/2
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/604054/diff/1/2#newcode1055
src/arm/stub-cache-arm.cc:1055: <<<<<<< .mine
On 2010/02/15 08:47:37, antonm wrote:
> merge conflict

Done.

http://codereview.chromium.org/604054/diff/1/2#newcode1065
src/arm/stub-cache-arm.cc:1065: CallInterceptorCompiler compiler(arguments());
On 2010/02/15 08:47:37, antonm wrote:
> 1) probably the whole compiler should go away as it now only have one method;
> 2) I am slightly concerned that various platform now diverge---could we bring
> the similar change to other platforms?

The compiler class has gone away.

We could bring the change to other platforms, when there is time.

[antonm, 2010-02-15 10:42:31]

LGTM.

Will you file a but to bring this change to other archs?

[Kevin Millikin (Chromium), 2010-02-15 10:58:50]

It needs just a bit of clean up.  LGTM if comments are addressed.

http://codereview.chromium.org/604054/diff/3001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/604054/diff/3001/1002#newcode979
src/arm/stub-cache-arm.cc:979: Register scratch1 = r2;
Why not call this something like "holder_reg"?

http://codereview.chromium.org/604054/diff/3001/1002#newcode988
src/arm/stub-cache-arm.cc:988: // Get the receiver from the stack into r0.
You should probably change the comments to not mention specific registers.

http://codereview.chromium.org/604054/diff/3001/1002#newcode998
src/arm/stub-cache-arm.cc:998: CheckPrototypes(JSObject::cast(object), receiver,
holder,
It's not clear in this function that object can be cast to a JSObject.  If it's
a precondition of the function, change the type of the parameter to JSObject*.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1002
src/arm/stub-cache-arm.cc:1002: reg = scratch1;
Don't bother assigning to reg, it's (effectively) dead since you use scratch1
(almost) consistently below.  Change the one use of reg below and get rid of
this assignment.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1014
src/arm/stub-cache-arm.cc:1014: JSFunction* function = 
lookup.GetConstantFunction();
Extra space after =.  It seems strange to name this when it's only used once in
the body and that use is so far from the name.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1018
src/arm/stub-cache-arm.cc:1018: __ push(scratch1);  // Save the holder.
__ push(holder_reg)

http://codereview.chromium.org/604054/diff/3001/1002#newcode1023
src/arm/stub-cache-arm.cc:1023: scratch1,
holder_reg

http://codereview.chromium.org/604054/diff/3001/1002#newcode1028
src/arm/stub-cache-arm.cc:1028: __ pop(scratch1);  // Restore the holder.
holder_reg

http://codereview.chromium.org/604054/diff/3001/1002#newcode1050
src/arm/stub-cache-arm.cc:1050: // Save the name_ register across the call.
This comment doesn't make sense (there is no "name_").

http://codereview.chromium.org/604054/diff/3001/1002#newcode1063
src/arm/stub-cache-arm.cc:1063: // Restore the name_ register.
This comment doesn't make sense (there is no "name_").  I'm not sure there needs
to be a comment for __ pop(name_reg).

[William Hesse, 2010-02-15 11:49:45]

http://codereview.chromium.org/604054/diff/3001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/604054/diff/3001/1002#newcode979
src/arm/stub-cache-arm.cc:979: Register scratch1 = r2;
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> Why not call this something like "holder_reg"?

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode988
src/arm/stub-cache-arm.cc:988: // Get the receiver from the stack into r0.
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> You should probably change the comments to not mention specific registers.

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode998
src/arm/stub-cache-arm.cc:998: CheckPrototypes(JSObject::cast(object), receiver,
holder,
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> It's not clear in this function that object can be cast to a JSObject.  If
it's
> a precondition of the function, change the type of the parameter to JSObject*.

This is a general issue on ComputeCallInterceptor and ComputeCallField, across
all platforms, with a TODO and a bug filed already.  This should be a separate
change.

There is code in the caller of this function that replaces object with the
holder of value types, so the cast succeeds.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1002
src/arm/stub-cache-arm.cc:1002: reg = scratch1;
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> Don't bother assigning to reg, it's (effectively) dead since you use scratch1
> (almost) consistently below.  Change the one use of reg below and get rid of
> this assignment.

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1014
src/arm/stub-cache-arm.cc:1014: JSFunction* function = 
lookup.GetConstantFunction();
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> Extra space after =.  It seems strange to name this when it's only used once
in
> the body and that use is so far from the name.

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1018
src/arm/stub-cache-arm.cc:1018: __ push(scratch1);  // Save the holder.
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> __ push(holder_reg)

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1050
src/arm/stub-cache-arm.cc:1050: // Save the name_ register across the call.
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> This comment doesn't make sense (there is no "name_").

Done.

http://codereview.chromium.org/604054/diff/3001/1002#newcode1063
src/arm/stub-cache-arm.cc:1063: // Restore the name_ register.
On 2010/02/15 10:58:50, Kevin Millikin wrote:
> This comment doesn't make sense (there is no "name_").  I'm not sure there
needs
> to be a comment for __ pop(name_reg).

Done.

[Kevin Millikin (Chromium), 2010-02-15 12:27:58]

http://codereview.chromium.org/604054/diff/3001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/604054/diff/3001/1002#newcode998
src/arm/stub-cache-arm.cc:998: CheckPrototypes(JSObject::cast(object), receiver,
holder,
On 2010/02/15 11:49:45, William Hesse wrote:
> This is a general issue on ComputeCallInterceptor and ComputeCallField, across
> all platforms, with a TODO and a bug filed already.  This should be a separate
> change.

It's about 8 lines of code---hardly warrants a separate change.