Issue 10698153: Change comparison-to-branch fusion to actually remove comparison from the graph.

Issue 10698153: Change comparison-to-branch fusion to actually remove comparison from the graph. (Closed)

Created:
8 years, 5 months ago by Vyacheslav Egorov (Google)

Modified:
8 years, 5 months ago

Reviewers:
srdjan

CC:
reviews_dartlang.org, vm-dev_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Change comparison-to-branch fusion to actually remove comparison from the graph. BUG= TEST= Committed: https://code.google.com/p/dart/source/detail?r=9556

Patch Set 1 #

Total comments: 4

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+54 lines, -81 lines)			Patch
M	runtime/vm/flow_graph_builder.cc	View	2 chunks	+5 lines, -1 line	0 comments	Download
M	runtime/vm/flow_graph_compiler.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/flow_graph_optimizer.cc	View	3 chunks	+12 lines, -3 lines	4 comments	Download
M	runtime/vm/il_printer.cc	View	1 chunk	+5 lines, -5 lines	0 comments	Download
M	runtime/vm/intermediate_language.h	View	3 chunks	+7 lines, -5 lines	0 comments	Download
M	runtime/vm/intermediate_language.cc	View	5 chunks	+11 lines, -11 lines	0 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	5 chunks	+4 lines, -16 lines	0 comments	Download
M	runtime/vm/intermediate_language_x64.cc	View	5 chunks	+4 lines, -16 lines	0 comments	Download
M	runtime/vm/locations.h	View	3 chunks	+3 lines, -19 lines	0 comments	Download
M	runtime/vm/locations.cc	View	1 chunk	+2 lines, -4 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Google)

PTAL. Cleanup of branch fusion ugliness.

8 years, 5 months ago (2012-07-11 14:51:59 UTC) #1

srdjan

LGTM! (In future we may consider replacing the BranchInstructions with a BranchCompareInstuction, but this solution ...

8 years, 5 months ago (2012-07-11 16:19:38 UTC) #2

Vyacheslav Egorov (Google)

8 years, 5 months ago (2012-07-11 17:30:26 UTC) #3

I am going to land as is.

https://chromiumcodereview.appspot.com/10698153/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_optimizer.cc (right):

https://chromiumcodereview.appspot.com/10698153/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:544: instr->previous()->set_next(branch);
On 2012/07/11 16:19:38, srdjan wrote:
> How about factoring this out into a Remove method? You should also clear next
> and previous of comparison so that it does nto accidentally get iterated.

It can't be factored into generic remove method because generic would have to
handle corner cases like removal of last instruction of the block when implicit
goto is present.  [last_instruction of the block has to be updated]. I
intentionally decided to remove it manually here because we know that it is not
the last instruction in the block.

https://chromiumcodereview.appspot.com/10698153/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_optimizer.cc:563: instr->previous()->set_next(branch);
On 2012/07/11 16:19:38, srdjan wrote:
> Ditto. Clear next/previous of branch and negate.

Currently we fuse from inside visiting of comparison. Iterator will crash when
trying to access next field. Teaching it to cope with that proves difficult
(also moving fusion to visiting of the branch is a major rewrite, I just tried).

Expand Messages | Collapse Messages