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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6691057: ARM: Add support load/store multiple VFP registers (Closed)

Created:
9 years, 8 months ago by Søren Thygesen Gjesse
Modified:
9 years, 7 months ago
CC:
v8-dev
Visibility:
Public.

Description

ARM: Add support load/store multiple VFP registers Enter/exit frames with save doubles use these instructions instead of generating 16 load/store instructions. R=karlklose@chromium.org, rodolph.perfetta@gmail.com BUG= TEST= Committed: http://code.google.com/p/v8/source/detail?r=7509

Patch Set 1 #

Patch Set 2 : Minor fixes #

Total comments: 46

Patch Set 3 : Comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+628 lines, -55 lines) Patch
M src/arm/assembler-arm.h View 1 2 2 chunks +25 lines, -1 line 0 comments Download
M src/arm/assembler-arm.cc View 1 2 2 chunks +83 lines, -1 line 0 comments Download
M src/arm/constants-arm.h View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M src/arm/disasm-arm.cc View 1 2 4 chunks +49 lines, -17 lines 0 comments Download
M src/arm/macro-assembler-arm.cc View 2 chunks +11 lines, -11 lines 0 comments Download
M src/arm/simulator-arm.h View 1 2 2 chunks +7 lines, -1 line 0 comments Download
M src/arm/simulator-arm.cc View 1 2 6 chunks +92 lines, -19 lines 0 comments Download
M test/cctest/test-assembler-arm.cc View 1 2 3 chunks +339 lines, -2 lines 0 comments Download
M test/cctest/test-disasm-arm.cc View 1 2 2 chunks +18 lines, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
Søren Thygesen Gjesse
Karl: All of it. Rodolph: Please double-check that the instruction is generated correctly. In GDB ...
9 years, 8 months ago (2011-04-05 15:02:51 UTC) #1
Rodolph Perfetta
Encoding is fine. I entered a few comments too. fldmias is the old (pre unified ...
9 years, 8 months ago (2011-04-05 18:16:39 UTC) #2
Karl Klose
LGTM if you address Rodolph's comments. Please update the year in the copyright text. http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc ...
9 years, 8 months ago (2011-04-06 06:52:55 UTC) #3
Søren Thygesen Gjesse
9 years, 8 months ago (2011-04-06 08:00:09 UTC) #4 
Comments addressed.

Added test for ia, db_w as well.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:35: // Copyright 2010 the V8 project authors. All
rights reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2044: // cond(31-28) | 110(27-25)| PUDW1(26-20) |
Rbase(19-16) |
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> PUDW1 is bit 24-20.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2045: // first(15-12) | 1010(11-8) | (count/2)
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> It should be count*2.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2048: ASSERT(am == ia || am == ia_w || am == db_w);
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> if the base is r15 and writeback is on, the instruction is undefined, an
assert
> would be good.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2062: Condition cond) {
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> same as above.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2081: Condition cond) {
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> same as above.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:2101: Condition cond) {
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> same as above.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/assembler-arm.h#newc...
src/arm/assembler-arm.h:35: // Copyright 2010 the V8 project authors. All rights
reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/constants-arm.h#newc...
src/arm/constants-arm.h:1: // Copyright 2010 the V8 project authors. All rights
reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc#newcode1
src/arm/disasm-arm.cc:1: // Copyright 2010 the V8 project authors. All rights
reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc#newcod...
src/arm/disasm-arm.cc:386: if (format[0] == 'S') reg = reg << 1 |
instr->DValue();
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> Using instr->VFPDRegValue(kSinglePrecision) would simplify the code. Could
also
> be reused above.

Changed to use instr->VFPXRegValue (X=M|M|D) in all cases and refactored a bit.

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc#newcod...
src/arm/disasm-arm.cc:1282: default: {
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> VLDM/VSTM covers cases 0x4-0x7, 0x9, 0xB for single precision. Some cases are
> undefined instructions (P==U, W==0)

Changed to use 6 explicit case values and default as before.

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc#newcod...
src/arm/disasm-arm.cc:1287: bool to_arm_register = (instr->VLValue() == 0x1);
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> This variable has a confusing name. It sounds like you're using vldm to load
> into an ARM (rather than VFP) register.

Changed to to_vfp_regsiter.

http://codereview.chromium.org/6691057/diff/2001/src/arm/disasm-arm.cc#newcod...
src/arm/disasm-arm.cc:1325: default: {
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> VLDM/VSTM covers cases 0x4, 0x5, 0x9 for double precision in V8. The
difference
> between single and double precision cases is caused by V8 supporting 16
> D-registers (removing cases 0x6, 0x7, 0xB.)

Changed to use 3 explicit case values and default as before.

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.cc#new...
src/arm/simulator-arm.cc:1: // Copyright 2010 the V8 project authors. All rights
reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.cc#new...
src/arm/simulator-arm.cc:1583: vd = instr->VdValue() << 1 | instr->DValue();
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> Use instr->VFPDregValue(kSinglePrecision) here.

Used

  vd = instr->VFPDRegValue(precision);

before the if.

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.cc#new...
src/arm/simulator-arm.cc:3012: bool to_arm_register = (instr->VLValue() == 0x1);
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> Since the instruction encoding is given to HandleVList, is it worth passing
the
> L bit separately?

No - argument removed.

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.cc#new...
src/arm/simulator-arm.cc:3075: bool to_arm_register = (instr->VLValue() == 0x1);
On 2011/04/05 18:16:39, Rodolph Perfetta wrote:
> Ditto.

Done.

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.h
File src/arm/simulator-arm.h (right):

http://codereview.chromium.org/6691057/diff/2001/src/arm/simulator-arm.h#newc...
src/arm/simulator-arm.h:1: // Copyright 2009 the V8 project authors. All rights
reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-assembler-a...
File test/cctest/test-assembler-arm.cc (right):

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-assembler-a...
test/cctest/test-assembler-arm.cc:1: // Copyright 2010 the V8 project authors.
All rights reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-assembler-a...
test/cctest/test-assembler-arm.cc:43: typedef Object* (*F4)(void* p0, void* p1,
int p2, int p3, int p4);
On 2011/04/06 06:52:56, Karl Klose wrote:
> Please unify naming in F4 and F3.

Done.

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-assembler-a...
test/cctest/test-assembler-arm.cc:642: // single presision values around in
memory.
On 2011/04/06 06:52:56, Karl Klose wrote:
> presision -> precision

Done.

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-disasm-arm.cc
File test/cctest/test-disasm-arm.cc (right):

http://codereview.chromium.org/6691057/diff/2001/test/cctest/test-disasm-arm....
test/cctest/test-disasm-arm.cc:1: // Copyright 2007-2008 the V8 project authors.
All rights reserved.
On 2011/04/06 06:52:56, Karl Klose wrote:
> 2011

Done.

Powered by Google App Engine
This is Rietveld 408576698