Issue 2860049: Port prototype-call-stubs for normal objects (http://codereview.chromium.org/...

Issue 2860049: Port prototype-call-stubs for normal objects (http://codereview.chromium.org/... (Closed)

Created:
10 years, 5 months ago by SeRya

Modified:
9 years, 7 months ago

Reviewers:
Mads Ager (chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Port prototype-call-stubs for normal objects (http://codereview.chromium.org/2801018).

Patch Set 3 : '' #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+486 lines, -265 lines)			Patch
M	src/arm/macro-assembler-arm.h	View	1	1 chunk	+0 lines, -18 lines	0 comments	Download
M	src/arm/macro-assembler-arm.cc	View	1	1 chunk	+0 lines, -82 lines	0 comments	Download
M	src/arm/stub-cache-arm.cc	View	1	12 chunks	+247 lines, -20 lines	0 comments	Download
M	src/ia32/stub-cache-ia32.cc	View	1 2	1 chunk	+4 lines, -2 lines	0 comments	Download
M	src/ic.cc	View	1	1 chunk	+0 lines, -5 lines	0 comments	Download
M	src/x64/macro-assembler-x64.h	View	1	1 chunk	+0 lines, -18 lines	0 comments	Download
M	src/x64/macro-assembler-x64.cc	View	1	1 chunk	+0 lines, -95 lines	0 comments	Download
M	src/x64/stub-cache-x64.cc	View	1	12 chunks	+235 lines, -25 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Mads Ager (chromium)

http://codereview.chromium.org/2860049/diff/51001/52003 File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/2860049/diff/51001/52003#newcode108 src/arm/stub-cache-arm.cc:108: Remove blank line or have the blank line before ...

10 years, 5 months ago (2010-07-12 10:42:57 UTC) #2

SeRya

http://codereview.chromium.org/2860049/diff/51001/52003 File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/2860049/diff/51001/52003#newcode108 src/arm/stub-cache-arm.cc:108: On 2010/07/12 10:42:57, Mads Ager wrote: > Remove blank ...

10 years, 5 months ago (2010-07-12 11:47:44 UTC) #3

http://codereview.chromium.org/2860049/diff/51001/52003
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/2860049/diff/51001/52003#newcode108
src/arm/stub-cache-arm.cc:108: 
On 2010/07/12 10:42:57, Mads Ager wrote:
> Remove blank line or have the blank line before the comment.

Done.

http://codereview.chromium.org/2860049/diff/51001/52003#newcode115
src/arm/stub-cache-arm.cc:115: if (extra.is(no_reg)) {
On 2010/07/12 10:42:57, Mads Ager wrote:
> Add a
> 
> // Check that receiver is a JSObject.
> 
> comment?

Done.

http://codereview.chromium.org/2860049/diff/51001/52003#newcode132
src/arm/stub-cache-arm.cc:132: Register tmp1 = receiver, tmp2 = properties;
On 2010/07/12 10:42:57, Mads Ager wrote:
> Could you use two lines please?

Done.

http://codereview.chromium.org/2860049/diff/51001/52003#newcode138
src/arm/stub-cache-arm.cc:138: __ b(ne, miss_label);
On 2010/07/12 10:42:57, Mads Ager wrote:
> Remove, this is emitted after the conditional.

Done.

http://codereview.chromium.org/2860049/diff/51001/52003#newcode200
src/arm/stub-cache-arm.cc:200: // Restore the properties if their register was
occupied by the name.
On 2010/07/12 10:42:57, Mads Ager wrote:
> There is no conditional here. Either update the comment or make the reloading
of
> the properties array guarded by extra.is(no_reg)?

The comment was outdated. Fixed.

http://codereview.chromium.org/2860049/diff/51001/52008
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/2860049/diff/51001/52008#newcode94
src/x64/stub-cache-x64.cc:94: Register extra) {
On 2010/07/12 10:42:57, Mads Ager wrote:
> I would guess that you do not need any of the checks for extra on x64 and arm?
> You should have enough registers on those platforms to always pass in an extra
> register and just assert at the top that extra is not no_reg.

Actually there is no difference among all of these three platforms. no_reg value
is used only in CallInterceptorCompiler instantiated in
CallStubCompiler::CompileCallInterceptor, but it seems that in all these cases
an additional register could be added.

Probably it would worth to do it to simplify GenerateDictionaryNegativeLookup,
but I may be in a separate CL?

http://codereview.chromium.org/2860049/diff/51001/52008#newcode109
src/x64/stub-cache-x64.cc:109: __ CmpInstanceType(r0, FIRST_JS_OBJECT_TYPE);
On 2010/07/12 10:42:57, Mads Ager wrote:
> Add short "Check that receiver is a JSObject" comment here on all platforms?

Done.

http://codereview.chromium.org/2860049/diff/51001/52008#newcode146
src/x64/stub-cache-x64.cc:146: __ SmiToInteger64(index, FieldOperand(properties,
kCapacityOffset));
On 2010/07/12 10:42:57, Mads Ager wrote:
> SmiToInteger32?

Done.

http://codereview.chromium.org/2860049/diff/51001/52008#newcode147
src/x64/stub-cache-x64.cc:147: __ decq(index);
On 2010/07/12 10:42:57, Mads Ager wrote:
> decl?

Done.

Mads Ager (chromium)

LGTM I think the simplification of the code by requiring an extra register would be ...

10 years, 5 months ago (2010-07-12 12:30:00 UTC) #4

Fixed lint error.

Expand Messages | Collapse Messages