Issue 11262008: Remove push arguments and replace constants in FlowGraph::InlineCall.

Issue 11262008: Remove push arguments and replace constants in FlowGraph::InlineCall. (Closed)

Created:
8 years, 2 months ago by zerny-google

Modified:
8 years, 1 month ago

Reviewers:
Kevin Millikin (Google)

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Remove push arguments and replace constants in FlowGraph::InlineCall. R=kmillikin@google.com Committed: https://code.google.com/p/dart/source/detail?r=14468

Patch Set 2 : Srdjan's DBCs from previous CL #

Total comments: 6

Patch Set 3 : Review update. #

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+35 lines, -37 lines)			Patch
	M	runtime/vm/flow_graph.cc	View	1 2	2 chunks	+30 lines, -3 lines	0 comments	Download
	M	runtime/vm/flow_graph_inliner.cc	View	1 2	7 chunks	+5 lines, -34 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Kevin Millikin (Google)

LGTM with a couple of comments. http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc File runtime/vm/flow_graph.cc (right): http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc#newcode800 runtime/vm/flow_graph.cc:800: join->predecessors_[new_index + 1]->block_id()); ...

8 years, 1 month ago (2012-11-02 11:35:23 UTC) #2

zerny-google

8 years, 1 month ago (2012-11-02 12:37:31 UTC) #3

Thanks for the comments.

http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:800: join->predecessors_[new_index + 1]->block_id());
On 2012/11/02 11:35:23, kmillikin wrote:
> The continuation line should be indented 4 spaces.

Done.

http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:807: ASSERT(join->predecessors_[new_index -
1]->block_id() <
On 2012/11/02 11:35:23, kmillikin wrote:
> Same.

Done.

http://codereview.chromium.org/11262008/diff/1003/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:1011: if (constant == NULL ||
On 2012/11/02 11:35:23, kmillikin wrote:
> I wouldn't use continue to skip a single statement.  It seems simpler to
negate
> the condition and have:
> 
> if (constant != NULL &&
>     ((constant->input_use_list() != NULL) ||
>      (constant->env_use_list() != NULL))) {
>   constant->ReplaceUsesWith(...);
> }

That is simpler. Thanks for pointing that out.

Expand Messages | Collapse Messages