Issue 11028115: DoNumberTagD performance improvement

Issue 11028115: DoNumberTagD performance improvement (Closed)

Created:
8 years, 2 months ago by aberent

Modified:
8 years, 2 months ago

Reviewers:
danno

CC:
v8-dev

Base URL:
git://github.com/v8/v8.git@master

Visibility:
Public.

More Reviews

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 #

Created: 8 years, 2 months ago

Download [raw] [tar.bz2]

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

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

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 ...

8 years, 2 months ago (2012-10-10 12:27:53 UTC) #2

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 ...

8 years, 2 months ago (2012-10-11 13:03:57 UTC) #4

aberent

8 years, 2 months ago (2012-10-11 15:16:53 UTC) #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.

Expand Messages | Collapse Messages