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

Issue 9414003: Initial implementation of a flow-graph builder for Dart's AST. (Closed)

Created:
8 years, 10 months ago by Kevin Millikin (Chromium)
Modified:
8 years, 10 months ago
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Initial implementation of a flow-graph builder for Dart's AST. Visit the AST and generate an instruction (ie, not basic-block) flow graph. The flow graph for the simple function: main() { var f = 1; var n = 5; while (n > 0) { f = f * n; n = n - 1; } print(f); } is: 1: StoreLocal(f, #1) 2: StoreLocal(n, #5) 3: [join] 4: t0 <-LoadLocal(n) 5: t0 <-InstanceCall(>, t0, #0) 6: if t0 goto(7, 15) 7: [target] 8: t0 <-LoadLocal(f) 9: t1 <-LoadLocal(n) 10: t0 <-InstanceCall(*, t0, t1) 11: StoreLocal(f, t0) 12: t0 <-LoadLocal(n) 13: t0 <-InstanceCall(-, t0, #1) 14: StoreLocal(n, t0) goto 3 15: [target] 16: t0 <-LoadLocal(f) 17: StaticCall(print, t0) 18: return #null BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=4460

Patch Set 1 #

Total comments: 20

Patch Set 2 : Big refactor to incorporate review comments. #

Total comments: 42

Patch Set 3 : Incorporated reviewer comments, did not rebase. #

Patch Set 4 : Rely on assumption that code can reach both branches of a condition. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1686 lines, -38 lines) Patch
M runtime/vm/ast.h View 1 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/code_patcher_ia32.cc View 1 4 chunks +6 lines, -6 lines 0 comments Download
M runtime/vm/compiler.cc View 1 4 chunks +9 lines, -1 line 0 comments Download
A runtime/vm/flow_graph_builder.h View 1 2 3 1 chunk +222 lines, -0 lines 0 comments Download
A runtime/vm/flow_graph_builder.cc View 1 2 3 1 chunk +893 lines, -0 lines 0 comments Download
M runtime/vm/handles_impl.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M runtime/vm/instructions_ia32.h View 1 5 chunks +14 lines, -14 lines 0 comments Download
M runtime/vm/instructions_ia32.cc View 1 3 chunks +6 lines, -6 lines 0 comments Download
M runtime/vm/instructions_ia32_test.cc View 1 3 chunks +4 lines, -4 lines 0 comments Download
A runtime/vm/intermediate_language.h View 1 2 3 1 chunk +328 lines, -0 lines 0 comments Download
A runtime/vm/intermediate_language.cc View 1 2 1 chunk +191 lines, -0 lines 0 comments Download
M runtime/vm/stack_frame_ia32.cc View 1 2 chunks +3 lines, -3 lines 0 comments Download
M runtime/vm/vm_sources.gypi View 1 3 chunks +6 lines, -2 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Kevin Millikin (Google)
Progress so far. All comments welcome.
8 years, 10 months ago (2012-02-16 09:10:30 UTC) #1
srdjan
https://chromiumcodereview.appspot.com/9414003/diff/1/runtime/vm/code_generator_ia32.cc File runtime/vm/code_generator_ia32.cc (right): https://chromiumcodereview.appspot.com/9414003/diff/1/runtime/vm/code_generator_ia32.cc#newcode207 runtime/vm/code_generator_ia32.cc:207: graph_builder.BuildGraph(); You want to decouple FlowGraphBuilder from old compiler/codegenerator. ...
8 years, 10 months ago (2012-02-16 14:14:33 UTC) #2
Kevin Millikin (Google)
https://chromiumcodereview.appspot.com/9414003/diff/1/runtime/vm/code_generator_ia32.cc File runtime/vm/code_generator_ia32.cc (right): https://chromiumcodereview.appspot.com/9414003/diff/1/runtime/vm/code_generator_ia32.cc#newcode207 runtime/vm/code_generator_ia32.cc:207: graph_builder.BuildGraph(); On 2012/02/16 14:14:34, srdjan wrote: > You want ...
8 years, 10 months ago (2012-02-20 14:50:19 UTC) #3
Kevin Millikin (Google)
New change list uploaded. 1. I renamed class Instruction in instructions_ia32.h to 'InstructionPattern' so it ...
8 years, 10 months ago (2012-02-20 14:54:45 UTC) #4
srdjan
Hi Kevin, Will grab the other CL and continue review there. Here is just the ...
8 years, 10 months ago (2012-02-21 14:00:21 UTC) #5
srdjan
Here are some more comments, will finish review today. Generally, I'd like to keep the ...
8 years, 10 months ago (2012-02-21 14:33:41 UTC) #6
srdjan
And the last bunch of comments. https://chromiumcodereview.appspot.com/9414003/diff/4001/runtime/vm/flow_graph_builder.cc File runtime/vm/flow_graph_builder.cc (right): https://chromiumcodereview.appspot.com/9414003/diff/4001/runtime/vm/flow_graph_builder.cc#newcode898 runtime/vm/flow_graph_builder.cc:898: if (bailout_reason_ == ...
8 years, 10 months ago (2012-02-22 00:41:34 UTC) #7
Kevin Millikin (Google)
I agree, I don't like bug changes like this. My defense is it's an initial ...
8 years, 10 months ago (2012-02-22 13:15:42 UTC) #8
srdjan
https://chromiumcodereview.appspot.com/9414003/diff/4001/runtime/vm/compiler.cc File runtime/vm/compiler.cc (right): https://chromiumcodereview.appspot.com/9414003/diff/4001/runtime/vm/compiler.cc#newcode122 runtime/vm/compiler.cc:122: // Currently, always fails and falls through to the ...
8 years, 10 months ago (2012-02-22 14:08:16 UTC) #9
srdjan
8 years, 10 months ago (2012-02-22 14:34:52 UTC) #10
Discussed remaining issues offline.

LGTM

Powered by Google App Engine
This is Rietveld 408576698