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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10384027: Wrap block-informations when embedding them in the graph. (Closed)

Created:
8 years, 7 months ago by Lasse Reichstein Nielsen
Modified:
8 years, 7 months ago
Reviewers:
karlklose, floitsch
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Wrap block-informations when embedding them in the graph. The following block is now stored in the wrapper, not in the block information, since its only relevant for the graph based iteration. Also split HLoopInformation into the original HLoopInformation and the block- based HLoopBlockInformation. Committed: https://code.google.com/p/dart/source/detail?r=7413

Patch Set 1 #

Total comments: 25

Patch Set 2 : Address review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+296 lines, -217 lines) Patch
M lib/compiler/implementation/ssa/bailout.dart View 1 3 chunks +9 lines, -8 lines 0 comments Download
M lib/compiler/implementation/ssa/builder.dart View 1 15 chunks +107 lines, -75 lines 0 comments Download
M lib/compiler/implementation/ssa/codegen.dart View 1 14 chunks +53 lines, -49 lines 0 comments Download
M lib/compiler/implementation/ssa/nodes.dart View 1 16 chunks +126 lines, -84 lines 0 comments Download
M lib/compiler/implementation/ssa/optimize.dart View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5 (0 generated)
Lasse Reichstein Nielsen
8 years, 7 months ago (2012-05-07 08:30:59 UTC) #1
Lasse Reichstein Nielsen
8 years, 7 months ago (2012-05-07 08:30:59 UTC) #2
Lasse Reichstein Nielsen
8 years, 7 months ago (2012-05-07 09:00:42 UTC) #3
karlklose
LGTM with a few comments. http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ssa/builder.dart File lib/compiler/implementation/ssa/builder.dart (right): http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ssa/builder.dart#newcode1421 lib/compiler/implementation/ssa/builder.dart:1421: HLoopBlockInformation loopBlockInfo = new ...
8 years, 7 months ago (2012-05-08 08:02:46 UTC) #4
Lasse Reichstein Nielsen
8 years, 7 months ago (2012-05-08 11:40:34 UTC) #5 
http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:1393: new HLabeledBlockInformation(
Made this easier to read too.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:1421: HLoopBlockInformation
loopBlockInfo = new HLoopBlockInformation(
On 2012/05/08 08:02:46, karlklose wrote:
> This constructor call is hard to read. Could you perhaps store subexpressions
in
> local variables and format the call like that in l. 1293?

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:1424: wrapExpressionGraph(new
SubExpression(conditionBlock,
On 2012/05/08 08:02:46, karlklose wrote:
> Store the subexpression in a local variable?

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:1426: wrapStatementGraph(new
SubGraph(bodyEntryBlock, bodyExitBlock)),
On 2012/05/08 08:02:46, karlklose wrote:
> Ditto for the subgraph?

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:1543: (elseGraph === null) ? null :
new HSubGraphBlockInformation(elseGraph));
On 2012/05/08 08:02:46, karlklose wrote:
> wrapStatmentGraph(elseGraph)

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:3097: catchGraph == null ? null
On 2012/05/08 08:02:46, karlklose wrote:
> use wrapStatementGraph

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:3099: finallyGraph == null ? null
On 2012/05/08 08:02:46, karlklose wrote:
> ditto.

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:426: HBlockFlow blockInformation =
null;
On 2012/05/08 08:02:46, karlklose wrote:
> Consider renaming this to blockFlow.

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:454: void
setBlockInfo(HBlockInformation blockInfo, HBasicBlock continuation) {
Changed to setBlockFlow.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:2168: // Non-block-based loop
information.
On 2012/05/08 08:02:46, karlklose wrote:
> Doc-style (/** ... *)?

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:2175: /** The corresponding block
information */
Yes. Missing a final '.' I can see.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:2217: 
On 2012/05/08 08:02:46, karlklose wrote:
> Remove line.

Done.

http://codereview.chromium.org/10384027/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/nodes.dart:2380: this.kind,
On 2012/05/08 08:02:46, karlklose wrote:
> Start parameters on the same line as constructor?

Done.

Powered by Google App Engine
This is Rietveld 408576698