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

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

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

Messages

Total messages: 6 (0 generated)
Kevin Millikin (Google)
2 years, 3 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.
2 years, 3 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 ...
2 years, 2 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 ...
2 years, 2 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 ...
2 years, 2 months ago (2013-05-03 11:47:38 UTC) #5
Kevin Millikin (Google)
2 years, 2 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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 5fa3ca5