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

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

Created:
7 years, 7 months ago by Kevin Millikin (Google)
Modified:
7 years, 7 months 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) Patch
M runtime/vm/compiler.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M runtime/vm/flow_graph.h View 2 chunks +4 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph.cc View 1 2 chunks +22 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_allocator.cc View 1 1 chunk +4 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_builder.h View 1 chunk +2 lines, -0 lines 0 comments Download
M runtime/vm/flow_graph_builder.cc View 1 6 chunks +12 lines, -5 lines 0 comments Download
M runtime/vm/flow_graph_compiler.h View 2 chunks +4 lines, -5 lines 0 comments Download
M runtime/vm/flow_graph_compiler.cc View 2 chunks +7 lines, -14 lines 0 comments Download
M runtime/vm/flow_graph_inliner.cc View 1 8 chunks +436 lines, -29 lines 0 comments Download
M runtime/vm/flow_graph_optimizer.h View 1 1 chunk +5 lines, -5 lines 0 comments Download
M runtime/vm/flow_graph_optimizer.cc View 1 5 chunks +42 lines, -3 lines 0 comments Download
M runtime/vm/flow_graph_type_propagator.cc View 1 2 chunks +15 lines, -0 lines 0 comments Download
M runtime/vm/il_printer.cc View 1 3 chunks +3 lines, -3 lines 0 comments Download
M runtime/vm/intermediate_language.h View 1 16 chunks +67 lines, -8 lines 0 comments Download
M runtime/vm/intermediate_language.cc View 1 2 chunks +20 lines, -5 lines 0 comments Download
M runtime/vm/intermediate_language_arm.cc View 1 5 chunks +14 lines, -4 lines 0 comments Download
M runtime/vm/intermediate_language_ia32.cc View 1 8 chunks +36 lines, -22 lines 0 comments Download
M runtime/vm/intermediate_language_mips.cc View 1 5 chunks +14 lines, -4 lines 0 comments Download
M runtime/vm/intermediate_language_x64.cc View 1 9 chunks +39 lines, -28 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Kevin Millikin (Google)
7 years, 7 months ago (2013-05-01 15:27:16 UTC) #1
Kevin Millikin (Google)
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.
7 years, 7 months ago (2013-05-01 15:36:24 UTC) #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 ...
7 years, 7 months ago (2013-05-02 20:06:30 UTC) #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 ...
7 years, 7 months ago (2013-05-03 10:41:36 UTC) #4
Kevin Millikin (Google)
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 ...
7 years, 7 months ago (2013-05-03 11:47:38 UTC) #5
Kevin Millikin (Google)
7 years, 7 months ago (2013-05-06 17:22:37 UTC) #6
Message was sent while issue was closed.
Committed patchset #2 manually as r22434 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698