Implement a custom call compiler for Array.pop.

Implement a custom call compiler for Array.pop.

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

[antonm, 2010-03-12 10:28:03]

Mads,

may you have a look?

[Mads Ager (chromium), 2010-03-12 10:57:35]

LGTM

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

http://codereview.chromium.org/870007/diff/1/2#newcode1344
src/ia32/stub-cache-ia32.cc:1344: Immediate(Factory::fixed_array_map()));
Indentation one space off?

http://codereview.chromium.org/870007/diff/1/2#newcode1357
src/ia32/stub-cache-ia32.cc:1357: __ j(equal, &call_builtin, not_taken);
I think it is fine to call the builtin in this case, but all you need to do is
return undefined in that case, so it would be easy to handle here as well.

Doing that, it seems we can remove the builtin once this is ported to the other
platforms.

[antonm, 2010-03-12 12:05:48]

Thanks a lot for review, Mads.

Doing stubs for other platforms and submitting.

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

http://codereview.chromium.org/870007/diff/1/2#newcode1344
src/ia32/stub-cache-ia32.cc:1344: Immediate(Factory::fixed_array_map()));
On 2010/03/12 10:57:37, Mads Ager wrote:
> Indentation one space off?

Done.

http://codereview.chromium.org/870007/diff/1/2#newcode1357
src/ia32/stub-cache-ia32.cc:1357: __ j(equal, &call_builtin, not_taken);
On 2010/03/12 10:57:37, Mads Ager wrote:
> I think it is fine to call the builtin in this case, but all you need to do is
> return undefined in that case, so it would be easy to handle here as well.
> 
> Doing that, it seems we can remove the builtin once this is ported to the
other
> platforms.

(as discussed off-line): we need it still to fetch data from proto chain if
present.

[Lasse Reichstein, 2010-03-12 12:31:37]

drive-by comments.

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

http://codereview.chromium.org/870007/diff/1/2#newcode1350
src/ia32/stub-cache-ia32.cc:1350: __ j(negative, &empty_array, not_taken);
not_taken is the default for forward jumps, but the implementation still emits
the hint. 
Either remove the hint here (defaulting to no_hint) or change
j(Condition,Label*,Hint) to not emit not_taken hints for forward jumps and taken
hints for backwards jumps.

http://codereview.chromium.org/870007/diff/1/2#newcode1352
src/ia32/stub-cache-ia32.cc:1352: // Get the last element.
Ensure that using times_half_pointer_size turns a smi into a pointer index,
e.g.,
STATIC_ASSERT(kSmiTagSize == 1);
STATIC_ASSERT(kSmiTag == 0);

http://codereview.chromium.org/870007/diff/1/2#newcode1357
src/ia32/stub-cache-ia32.cc:1357: __ j(equal, &call_builtin, not_taken);
Remove not_taken here too..

http://codereview.chromium.org/870007/diff/1/2#newcode1376
src/ia32/stub-cache-ia32.cc:1376: argc + 1,
Indentation.

[antonm, 2010-03-12 12:43:56]

Thanks a lot for comments, Lasse.

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

http://codereview.chromium.org/870007/diff/1/2#newcode1350
src/ia32/stub-cache-ia32.cc:1350: __ j(negative, &empty_array, not_taken);
On 2010/03/12 12:31:37, Lasse Reichstein wrote:
> not_taken is the default for forward jumps, but the implementation still emits
> the hint. 
> Either remove the hint here (defaulting to no_hint) or change
> j(Condition,Label*,Hint) to not emit not_taken hints for forward jumps and
taken
> hints for backwards jumps.

Fixed.  I just wanted to be consistent with the rest of the code, but yes, I
schedule my jumps that way :)

Let me change ArrayPush impl as well.

http://codereview.chromium.org/870007/diff/1/2#newcode1352
src/ia32/stub-cache-ia32.cc:1352: // Get the last element.
On 2010/03/12 12:31:37, Lasse Reichstein wrote:
> Ensure that using times_half_pointer_size turns a smi into a pointer index,
> e.g.,
> STATIC_ASSERT(kSmiTagSize == 1);
> STATIC_ASSERT(kSmiTag == 0);

Done.

http://codereview.chromium.org/870007/diff/1/2#newcode1357
src/ia32/stub-cache-ia32.cc:1357: __ j(equal, &call_builtin, not_taken);
On 2010/03/12 12:31:37, Lasse Reichstein wrote:
> Remove not_taken here too..

Sure.

http://codereview.chromium.org/870007/diff/1/2#newcode1376
src/ia32/stub-cache-ia32.cc:1376: argc + 1,
On 2010/03/12 12:31:37, Lasse Reichstein wrote:
> Indentation.

Done.