Chromium Code Reviews
Help | Chromium Project | Sign in
(401)

Issue 11028115: DoNumberTagD performance improvement (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by aberent
Modified:
1 year, 6 months ago
Reviewers:
danno
CC:
v8-dev_googlegroups.com
Base URL:
git://github.com/v8/v8.git@master
Visibility:
Public.

Description

DoNumberTagD performance improvement

Allocate heap entry untagged and tag at end to avoid having to subtract off
the tag offset before storing the value.

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12711

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fix review comments and implement further cases. #

Total comments: 4

Patch Set 3 : Fix remaining code review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -36 lines) Lint Patch
M src/arm/lithium-codegen-arm.cc View 1 2 4 chunks +11 lines, -6 lines 0 comments 0 errors Download
M src/arm/macro-assembler-arm.h View 1 2 2 chunks +9 lines, -1 line 0 comments 0 errors Download
M src/arm/macro-assembler-arm.cc View 1 2 2 chunks +9 lines, -3 lines 0 comments 0 errors Download
M src/arm/stub-cache-arm.cc View 1 2 6 chunks +29 lines, -26 lines 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 5
aberent
PTAL
1 year, 6 months ago #1
danno
LGTM with nits pending performance numbers http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc#newcode3134 src/arm/macro-assembler-arm.cc:3134: tagged?TAG_OBJECT:NO_ALLOCATION_FLAGS); Spaces between ...
1 year, 6 months ago #2
aberent
1 year, 6 months ago #3
danno
Almost there... http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc#newcode4692 src/arm/lithium-codegen-arm.cc:4692: ASSERT(input->Equals(instrt->result())); I think the above change is ...
1 year, 6 months ago #4
aberent
1 year, 6 months ago #5
http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.cc...
src/arm/macro-assembler-arm.cc:3134: tagged?TAG_OBJECT:NO_ALLOCATION_FLAGS);
On 2012/10/10 12:27:53, danno wrote:
> Spaces between ? and :

Done.

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

http://codereview.chromium.org/11028115/diff/1/src/arm/macro-assembler-arm.h#...
src/arm/macro-assembler-arm.h:735: bool tagged = true);
On 2012/10/10 12:27:53, danno wrote:
> Use an enum here, like "enum TaggingMode { TAG_RESULT, DONT_TAG_RESULT }" ,
> otherwise at the call site it's difficult to tell what the true/false actually
> means

Done.

http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:4692: ASSERT(input->Equals(instrt->result()));
On 2012/10/11 13:03:57, danno wrote:
> I think the above change is a typo.

Done.

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/11028115/diff/6001/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3809: 
On 2012/10/11 13:03:57, danno wrote:
> nit: remove introduced whitespace

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434