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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6879009: Support Float64Arrays (Closed)

Created:
9 years, 8 months ago by Jakob Kummerow
Modified:
9 years, 6 months ago
Reviewers:
Karl Klose, danno
CC:
v8-dev
Visibility:
Public.

Description

Support Float64Arrays BUG=None TEST=mjsunit/external-arrays.js; updated cctest; existing unit tests Committed: http://code.google.com/p/v8/source/detail?r=7675

Patch Set 1 #

Total comments: 16

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+600 lines, -206 lines) Patch
M include/v8.h View 2 chunks +2 lines, -1 line 0 comments Download
M samples/shell.cc View 3 chunks +9 lines, -0 lines 0 comments Download
M src/api.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M src/arm/assembler-arm.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/arm/code-stubs-arm.h View 1 1 chunk +143 lines, -0 lines 0 comments Download
M src/arm/code-stubs-arm.cc View 1 4 chunks +47 lines, -160 lines 0 comments Download
M src/arm/lithium-arm.cc View 1 2 chunks +10 lines, -4 lines 0 comments Download
M src/arm/lithium-codegen-arm.cc View 1 4 chunks +13 lines, -0 lines 0 comments Download
M src/arm/stub-cache-arm.cc View 1 6 chunks +73 lines, -1 line 0 comments Download
M src/heap.h View 3 chunks +3 lines, -0 lines 0 comments Download
M src/heap.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M src/hydrogen-instructions.h View 1 2 chunks +4 lines, -2 lines 0 comments Download
M src/hydrogen-instructions.cc View 1 2 chunks +6 lines, -0 lines 0 comments Download
M src/ia32/lithium-codegen-ia32.cc View 4 chunks +8 lines, -0 lines 0 comments Download
M src/ia32/lithium-ia32.cc View 1 2 chunks +10 lines, -4 lines 0 comments Download
M src/ia32/stub-cache-ia32.cc View 4 chunks +15 lines, -2 lines 0 comments Download
M src/objects.h View 5 chunks +32 lines, -0 lines 0 comments Download
M src/objects.cc View 1 17 chunks +57 lines, -8 lines 0 comments Download
M src/objects-debug.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M src/objects-inl.h View 5 chunks +30 lines, -2 lines 0 comments Download
M src/objects-printer.cc View 4 chunks +16 lines, -0 lines 0 comments Download
M src/objects-visiting.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime.cc View 2 chunks +26 lines, -16 lines 0 comments Download
M src/stub-cache.cc View 3 chunks +6 lines, -0 lines 0 comments Download
M src/x64/lithium-codegen-x64.cc View 1 4 chunks +8 lines, -0 lines 0 comments Download
M src/x64/lithium-x64.cc View 1 2 chunks +10 lines, -4 lines 0 comments Download
M src/x64/stub-cache-x64.cc View 4 chunks +13 lines, -1 line 0 comments Download
M test/cctest/test-api.cc View 1 4 chunks +14 lines, -1 line 0 comments Download
M test/mjsunit/external-array.js View 1 chunk +25 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Jakob Kummerow
Karl, can you please review the ARM changes? Danno, please take a look at the ...
9 years, 8 months ago (2011-04-18 13:50:28 UTC) #1
danno
LGTM for non-ARM parts http://codereview.chromium.org/6879009/diff/1/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/6879009/diff/1/src/objects-inl.h#newcode3669 src/objects-inl.h:3669: default: Take out the default ...
9 years, 8 months ago (2011-04-18 16:36:28 UTC) #2
Karl Klose
The ARM part LGTM. http://codereview.chromium.org/6879009/diff/1/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6879009/diff/1/src/arm/code-stubs-arm.cc#newcode524 src/arm/code-stubs-arm.cc:524: void FloatingPointHelper::ConvertSmiToDouble(MacroAssembler* masm, The name ...
9 years, 8 months ago (2011-04-19 06:24:56 UTC) #3
Jakob Kummerow
9 years, 8 months ago (2011-04-20 15:01:12 UTC) #4 
Karl: is the named constant what you had in mind?

http://codereview.chromium.org/6879009/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6879009/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:524: void
FloatingPointHelper::ConvertSmiToDouble(MacroAssembler* masm,
On 2011/04/19 06:24:57, Karl Klose wrote:
> The name is confusing, because the value is assumed to be untagged. I think
> something like ConvertInt32ToDouble would be better.

Done. I chose "ConvertIntToDouble" because elsewhere in the code "Int32" is used
for ints that are too large to fit into a Smi.

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

http://codereview.chromium.org/6879009/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:3529: __ ldr(r3, MemOperand(r4, 4));
On 2011/04/19 06:24:57, Karl Klose wrote:
> Can you use named constants here (as you did below)?

Done.

http://codereview.chromium.org/6879009/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:3858: __ str(r7, MemOperand(r3, 4));
On 2011/04/19 06:24:57, Karl Klose wrote:
> Ditto.

Done.

http://codereview.chromium.org/6879009/diff/1/src/arm/stub-cache-arm.cc#newco...
src/arm/stub-cache-arm.cc:4006: __ str(r5, MemOperand(r7, 4));
On 2011/04/19 06:24:57, Karl Klose wrote:
> Ditto.

Done.

http://codereview.chromium.org/6879009/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/6879009/diff/1/src/objects-inl.h#newcode3669
src/objects-inl.h:3669: default:
On 2011/04/18 16:36:28, danno wrote:
> Take out the default since you now cover all cases?

No. There's a bunch of other values in the enum InstanceType that are not
handled here (because they're not external array types).

http://codereview.chromium.org/6879009/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6879009/diff/1/src/objects.cc#newcode9266
src/objects.cc:9266: double cast_value = 0;
On 2011/04/18 16:36:28, danno wrote:
> probably don't want to call this cast_value since it's not casted.

Done.

http://codereview.chromium.org/6879009/diff/1/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/6879009/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:2355: Operand(external_pointer,  key, times_8,
0));
On 2011/04/18 16:36:28, danno wrote:
> remove extra space before key

Done.

http://codereview.chromium.org/6879009/diff/1/src/x64/lithium-codegen-x64.cc#...
src/x64/lithium-codegen-x64.cc:3045: __ movsd(Operand(external_pointer,  key,
times_8, 0),
On 2011/04/18 16:36:28, danno wrote:
> remove extra space before key

Done.

Powered by Google App Engine
This is Rietveld 408576698