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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11092090: Inline indexed load and store of typed array float64. (Closed)

Created:
8 years, 2 months ago by srdjan
Modified:
8 years, 2 months ago
Reviewers:
Florian Schneider
CC:
reviews_dartlang.org, john_johnmccutchan.com
Visibility:
Public.

Description

Inline indexed load and store of typed array float64. NavierStokes time improves from 34,775us to 12,000us (v8 runs NavierStokes in 16000 us). Committed: https://code.google.com/p/dart/source/detail?r=13682

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Total comments: 10

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+422 lines, -154 lines) Patch
M runtime/vm/flow_graph_compiler.cc View 1 2 3 4 5 6 7 8 9 1 chunk +23 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.h View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.cc View 1 2 3 4 5 6 7 8 9 1 chunk +18 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_x64.h View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler_x64.cc View 1 2 3 4 5 6 7 8 9 1 chunk +18 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_optimizer.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_optimizer.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +113 lines, -63 lines 0 comments Download
M runtime/vm/intermediate_language.h View 1 2 3 4 5 6 7 8 9 10 chunks +35 lines, -14 lines 0 comments Download
M runtime/vm/intermediate_language.cc View 1 2 3 4 5 6 7 8 9 1 chunk +63 lines, -1 line 0 comments Download
M runtime/vm/intermediate_language_ia32.cc View 1 2 3 4 5 6 7 8 9 3 chunks +55 lines, -36 lines 0 comments Download
M runtime/vm/intermediate_language_x64.cc View 1 2 3 4 5 6 7 8 9 3 chunks +55 lines, -39 lines 0 comments Download
M tests/standalone/float_array_test.dart View 1 2 3 4 5 6 7 8 9 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
srdjan
8 years, 2 months ago (2012-10-12 19:53:55 UTC) #1
Florian Schneider
LGTM. https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_compiler.cc File runtime/vm/flow_graph_compiler.cc (right): https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_compiler.cc#newcode912 runtime/vm/flow_graph_compiler.cc:912: offset * (1 << TIMES_8) + Float64Array::data_offset(); Alternatively ...
8 years, 2 months ago (2012-10-15 09:40:25 UTC) #2
srdjan
Thanks, will commit once M1 is done. https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_compiler.cc File runtime/vm/flow_graph_compiler.cc (right): https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_compiler.cc#newcode912 runtime/vm/flow_graph_compiler.cc:912: offset * ...
8 years, 2 months ago (2012-10-15 16:53:13 UTC) #3
srdjan
8 years, 2 months ago (2012-10-15 21:24:13 UTC) #4 
Fixed a bug: must type check value for store indexed (must be double). Added
test

On 2012/10/15 16:53:13, srdjan wrote:
> Thanks, will commit once M1 is done.
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> File runtime/vm/flow_graph_compiler.cc (right):
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> runtime/vm/flow_graph_compiler.cc:912: offset * (1 << TIMES_8) +
> Float64Array::data_offset();
> On 2012/10/15 09:40:25, Florian Schneider wrote:
> > Alternatively use kDoubleSize instead of 1 << TIMES_8.
> 
> Done.
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> File runtime/vm/flow_graph_compiler_ia32.cc (right):
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> runtime/vm/flow_graph_compiler_ia32.cc:36: case kGrowableObjectArrayCid:
> On 2012/10/15 09:40:25, Florian Schneider wrote:
> > I'd rather omit GrowableObjectArray here. This case should not occur since
> since
> > loads/stores only occur on the backing store which is an plain array.
> 
> Changed the generation of LoadIndexed to use the proper class id (changes
> PrepareIndexedOp). Removed GrowableObjectArray here and X64.
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> File runtime/vm/flow_graph_compiler_x64.cc (right):
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/flow_graph_com...
> runtime/vm/flow_graph_compiler_x64.cc:36: case kGrowableObjectArrayCid:
> On 2012/10/15 09:40:25, Florian Schneider wrote:
> > Omit GrowableObjectArray case here (see ia32 comment)
> 
> Done.
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/intermediate_l...
> File runtime/vm/intermediate_language_ia32.cc (right):
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/intermediate_l...
> runtime/vm/intermediate_language_ia32.cc:2221: length_offset =
> ByteArray::length_offset();
> On 2012/10/15 09:40:25, Florian Schneider wrote:
> > Even though length_offset is defined in the superclass ByteArray, maybe
write
> > Float64Array::length_offset() for consistency?
> 
> Done.
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/intermediate_l...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://codereview.chromium.org/11092090/diff/27001/runtime/vm/intermediate_l...
> runtime/vm/intermediate_language_x64.cc:2188: length_offset =
> ByteArray::length_offset();
> On 2012/10/15 09:40:25, Florian Schneider wrote:
> > Float64Array::length_offset()?
> 
> Done.

Powered by Google App Engine
This is Rietveld 408576698