Chromium Code Reviews
Help | Chromium Project | Sign in
(27)

Issue 14740005: Initial support for polymorphic inlining. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 3 weeks ago by kmillikin
Modified:
11 months, 3 weeks ago
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Initial support for polymorphic inlining.

Consider each polymorphic variant separately for inlining in frequency
order. Share inlined bodies for shared targets.

Insert an SSA redefinition of the receiver in each inlined variant to
prevent hoisting. Hoisting code specialized to the receiver (e.g.,
direct access to internal fields of typed data arrays) out of the
inlined body is not safe.

R=fschneider@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=22434

Patch Set 1 #

Total comments: 21

Patch Set 2 : Incorporated review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+747 lines, -137 lines) Lint Patch
M runtime/vm/compiler.cc View 1 1 chunk +1 line, -0 lines 0 comments ? errors Download
M runtime/vm/flow_graph.h View 2 chunks +4 lines, -0 lines 0 comments ? errors Download
M runtime/vm/flow_graph.cc View 1 2 chunks +22 lines, -1 line 0 comments ? errors Download
M runtime/vm/flow_graph_allocator.cc View 1 1 chunk +4 lines, -1 line 0 comments 0 errors Download
M runtime/vm/flow_graph_builder.h View 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M runtime/vm/flow_graph_builder.cc View 1 6 chunks +12 lines, -5 lines 0 comments ? errors Download
M runtime/vm/flow_graph_compiler.h View 2 chunks +4 lines, -5 lines 0 comments ? errors Download
M runtime/vm/flow_graph_compiler.cc View 2 chunks +7 lines, -14 lines 0 comments 0 errors Download
M runtime/vm/flow_graph_inliner.cc View 1 8 chunks +436 lines, -29 lines 0 comments ? errors Download
M runtime/vm/flow_graph_optimizer.h View 1 1 chunk +5 lines, -5 lines 0 comments ? errors Download
M runtime/vm/flow_graph_optimizer.cc View 1 5 chunks +42 lines, -3 lines 0 comments ? errors Download
M runtime/vm/flow_graph_type_propagator.cc View 1 2 chunks +15 lines, -0 lines 0 comments ? errors Download
M runtime/vm/il_printer.cc View 1 3 chunks +3 lines, -3 lines 0 comments ? errors Download
M runtime/vm/intermediate_language.h View 1 16 chunks +67 lines, -8 lines 0 comments ? errors Download
M runtime/vm/intermediate_language.cc View 1 2 chunks +20 lines, -5 lines 0 comments ? errors Download
M runtime/vm/intermediate_language_arm.cc View 1 5 chunks +14 lines, -4 lines 0 comments ? errors Download
M runtime/vm/intermediate_language_ia32.cc View 1 8 chunks +36 lines, -22 lines 0 comments ? errors Download
M runtime/vm/intermediate_language_mips.cc View 1 5 chunks +14 lines, -4 lines 0 comments ? errors Download
M runtime/vm/intermediate_language_x64.cc View 1 9 chunks +39 lines, -28 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 6
kmillikin
11 months, 3 weeks ago #1
kmillikin
https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph_allocator.cc File runtime/vm/flow_graph_allocator.cc (right): https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph_allocator.cc#newcode2191 runtime/vm/flow_graph_allocator.cc:2191: ASSERT(false); This is unintentional. I will remove it.
11 months, 3 weeks ago #2
srdjan
LGTM with Florian's LGTM. Please run the tests with optimization-counter-threshold=5 https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph_inliner.cc File runtime/vm/flow_graph_inliner.cc (right): https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph_inliner.cc#newcode978 ...
11 months, 3 weeks ago #3
Florian Schneider
LGTM. https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph.cc#newcode850 runtime/vm/flow_graph.cc:850: void FlowGraph::RemoveRedefinitions() { This is very similar to ...
11 months, 3 weeks ago #4
kmillikin
Thanks for the review. https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): https://codereview.chromium.org/14740005/diff/1/runtime/vm/flow_graph.cc#newcode850 runtime/vm/flow_graph.cc:850: void FlowGraph::RemoveRedefinitions() { On 2013/05/03 ...
11 months, 3 weeks ago #5
kmillikin
11 months, 3 weeks ago #6
Message was sent while issue was closed.
Committed patchset #2 manually as r22434 (presubmit successful).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6