Issue 10536145: Fuse comparisons that are used by branches together.

Issue 10536145: Fuse comparisons that are used by branches together. (Closed)

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

Modified:
8 years, 6 months ago

Reviewers:
srdjan

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Fuse comparisons that are used by branches together. This allows us to eliminate materialization of boolean value. Fix a bug in ToDoubleComp::LocationSummary on ia32. R=srdjan@google.com Committed: https://code.google.com/p/dart/source/detail?r=8617

Patch Set 1 #

Total comments: 11

Patch Set 2 : address review comments #

Created: 8 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+336 lines, -173 lines)			Patch
M	runtime/vm/flow_graph_compiler.cc	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	runtime/vm/flow_graph_compiler_ia32.h	View	1	2 chunks	+6 lines, -0 lines	0 comments	Download
M	runtime/vm/flow_graph_compiler_ia32.cc	View	1	3 chunks	+5 lines, -8 lines	0 comments	Download
M	runtime/vm/flow_graph_compiler_x64.h	View	1	2 chunks	+6 lines, -0 lines	0 comments	Download
M	runtime/vm/flow_graph_compiler_x64.cc	View	1	3 chunks	+5 lines, -8 lines	0 comments	Download
M	runtime/vm/flow_graph_optimizer.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	runtime/vm/flow_graph_optimizer.cc	View		2 chunks	+37 lines, -0 lines	0 comments	Download
M	runtime/vm/il_printer.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	runtime/vm/intermediate_language.h	View	1	7 chunks	+52 lines, -23 lines	0 comments	Download
M	runtime/vm/intermediate_language.cc	View	1	3 chunks	+65 lines, -31 lines	0 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	1	11 chunks	+69 lines, -39 lines	0 comments	Download
M	runtime/vm/intermediate_language_x64.cc	View	1	10 chunks	+82 lines, -63 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Google)

Currently done in a very hacky way. Ideally I would like to eradicate fused comparison ...

8 years, 6 months ago (2012-06-13 12:54:31 UTC) #2

srdjan

LGTM with comments http://codereview.chromium.org/10536145/diff/1/runtime/vm/flow_graph_compiler_ia32.h File runtime/vm/flow_graph_compiler_ia32.h (right): http://codereview.chromium.org/10536145/diff/1/runtime/vm/flow_graph_compiler_ia32.h#newcode270 runtime/vm/flow_graph_compiler_ia32.h:270: const Bool& false_value_; I find the ...

8 years, 6 months ago (2012-06-13 15:29:35 UTC) #3

Vyacheslav Egorov (Google)

8 years, 6 months ago (2012-06-13 16:23:43 UTC) #4

landed

http://codereview.chromium.org/10536145/diff/1/runtime/vm/flow_graph_compiler...
File runtime/vm/flow_graph_compiler_ia32.h (right):

http://codereview.chromium.org/10536145/diff/1/runtime/vm/flow_graph_compiler...
runtime/vm/flow_graph_compiler_ia32.h:270: const Bool& false_value_;
On 2012/06/13 15:29:35, srdjan wrote:
> I find the name 'true_value' ambiguous as it does no specify what bool value
it
> is (C, Dart, some enum). Better name are, IMO, true_object, bool_true,
> true_handle. The C++ code uses Bool::True() to get the RawObject.

Done.

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
File runtime/vm/intermediate_language.cc (right):

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
runtime/vm/intermediate_language.cc:743: if (is_fused_with_comparison()) {
On 2012/06/13 15:29:35, srdjan wrote:
> Add comment: no code needs to be emitted.

Done.

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
runtime/vm/intermediate_language.cc:841: if (fused_with_branch() == NULL) {
On 2012/06/13 15:29:35, srdjan wrote:
> !is_fused_with_branch() or revert the if clauses to think positive.

Done.

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
File runtime/vm/intermediate_language.h (right):

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
runtime/vm/intermediate_language.h:518: }
On 2012/06/13 15:29:35, srdjan wrote:
> Maybe also is_fused_with_branch => fused_with_branch_ != NULL, then add an
> assert to fused_with_branch that it must not be NULL.

Done.

http://codereview.chromium.org/10536145/diff/1/runtime/vm/intermediate_langua...
runtime/vm/intermediate_language.h:2212: bool fused_with_comparison_;
On 2012/06/13 15:29:35, srdjan wrote:
> rename to is_fused_with_comparison_

Done.

Expand Messages | Collapse Messages