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

Issue 9553008: Implement DoWhile. (Closed)

Created:
8 years, 9 months ago by srdjan
Modified:
8 years, 9 months ago
CC:
reviews_dartlang.org, vm-dev_dartlang.org
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -11 lines) Patch
M runtime/vm/flow_graph_builder.h View 1 2 3 2 chunks +4 lines, -4 lines 1 comment Download
M runtime/vm/flow_graph_builder.cc View 1 2 3 5 chunks +28 lines, -1 line 2 comments Download
M runtime/vm/intermediate_language.h View 1 2 3 2 chunks +6 lines, -6 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
srdjan
8 years, 9 months ago (2012-03-06 00:52:14 UTC) #1
Kevin Millikin (Google)
8 years, 9 months ago (2012-03-07 09:15:44 UTC) #2
LGTM.

https://chromiumcodereview.appspot.com/9553008/diff/6001/runtime/vm/flow_grap...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/9553008/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:452: // Tie do-while loop (test is after the
body).
I think we can come up with a generic loop-tying function, for all loop forms. 
It is parameterized over two edges: entry to the whole loop (either entry to
test for test at the top, or entry to body for test at the bottom), optional
back edge (body exit for test at the top, true successor of test for test at the
bottom).  It takes a partially tied loop and does the rest.

Let's keep that in mind for later.  There isn't really much duplicated code
between the two kinds of loop right now, but there will be for break and
continue handling.

https://chromiumcodereview.appspot.com/9553008/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.cc:456: Instruction* body_exit =
for_body.is_empty() ? join : for_body.exit();
This is correct but contains introduces an extra basic block in the case where
the body is not empty.  OK to leave it as is for now (especially since we
haven't sorted out continue), but it can be eliminated if we want later.

https://chromiumcodereview.appspot.com/9553008/diff/6001/runtime/vm/flow_grap...
File runtime/vm/flow_graph_builder.h (right):

https://chromiumcodereview.appspot.com/9553008/diff/6001/runtime/vm/flow_grap...
runtime/vm/flow_graph_builder.h:212: BlockEntryInstr** true_successor_address()
const {
The reason this was TargetEntry is to statically enforce that the graph contains
no critical edges.  That's a useful property to have (for dataflow analysis and
for translation out of SSA).

Of course, that means that you have to eagerly split the back edge for the
do/while loop; but then you eliminate the need a pass to convert and code to do
runtime verification in debug builds.

OK to keep it like this for do/while, and add it to the list of issues to
settle.

Powered by Google App Engine
This is Rietveld 408576698