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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years ago by Kevin Millikin (Google)
Modified:
2 years 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
Trybot results:
Commit: CQ not working?

Messages

Total messages: 6 (0 generated)
Kevin Millikin (Google)
2 years 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.
2 years 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 ...
2 years 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 ...
2 years 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 ...
2 years ago (2013-05-03 11:47:38 UTC) #5
Kevin Millikin (Google)
2 years ago (2013-05-06 17:22:37 UTC) #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 ec887be