Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(346)

Issue 7639015: ARM: Optimize stubs for incremental marking. (Closed)

Created:
9 years, 4 months ago by Michael Starzinger
Modified:
9 years, 4 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

ARM: Optimize stubs for incremental marking. R=erik.corry@gmail.com BUG=v8:1456 Committed: http://code.google.com/p/v8/source/detail?r=8954

Patch Set 1 #

Patch Set 2 : Fix readability issue. #

Total comments: 8

Patch Set 3 : Incorporated review by Erik Corry. #

Total comments: 4

Patch Set 4 : Incorporated second review by Erik Corry. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -14 lines) Patch
M src/arm/code-stubs-arm.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/arm/code-stubs-arm.cc View 1 2 4 chunks +50 lines, -5 lines 0 comments Download
M src/arm/full-codegen-arm.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/arm/macro-assembler-arm.h View 2 chunks +3 lines, -3 lines 0 comments Download
M src/arm/macro-assembler-arm.cc View 1 2 3 3 chunks +97 lines, -4 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
Michael Starzinger
9 years, 4 months ago (2011-08-12 09:53:10 UTC) #1
Erik Corry
LGTM http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc#newcode6785 src/arm/code-stubs-arm.cc:6785: ne, Seems like you should make this 'eq', ...
9 years, 4 months ago (2011-08-16 09:29:23 UTC) #2
Michael Starzinger
Added new patch set. http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7639015/diff/1006/src/arm/code-stubs-arm.cc#newcode6785 src/arm/code-stubs-arm.cc:6785: ne, On 2011/08/16 09:29:23, Erik ...
9 years, 4 months ago (2011-08-16 15:31:05 UTC) #3
Erik Corry
Still LGTM http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm.cc#newcode3353 src/arm/macro-assembler-arm.cc:3353: tst(instance_type, Operand(kStringEncodingMask)); Please move the tst to ...
9 years, 4 months ago (2011-08-17 07:56:25 UTC) #4
Michael Starzinger
9 years, 4 months ago (2011-08-17 08:57:15 UTC) #5
Added new patch set.

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3353: tst(instance_type,
Operand(kStringEncodingMask));
On 2011/08/17 07:56:25, Erik Corry wrote:
> Please move the tst to be adjacent to the conditional mov.  This is clearer
and
> probably faster as the load has a minimum latency of two cycles so it needs to
> be started earlier, at least on older CPUs that cannot lift it automatically.

Done. I had to switch to the IP register, because instance_type and length are
both aliased to the same register.

http://codereview.chromium.org/7639015/diff/5001/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3359: mov(length, Operand(length, LSR,
kSmiTagSize), LeaveCC, ne);
On 2011/08/17 07:56:25, Erik Corry wrote:
> Instead of kSmiTagSize I would suggest just '1' since the shift distance here
> does not correspond to the size of the tag.  It's the difference between the
> size of the tag and the log2 of the sizeof a UC16 character.  But that seems a
> little complicated to express in code, so I suggest just writing '1' in
> conjunction with the asserts you already have.

Done.

Powered by Google App Engine
This is Rietveld 408576698