Issue 11273111: Allow bound check elimination to eliminate checks when both array length and index boundaries are e…

Issue 11273111: Allow bound check elimination to eliminate checks when both array length and index boundaries are e… (Closed)

Created:
8 years, 1 month ago by Vyacheslav Egorov (Google)

Modified:
8 years, 1 month ago

Reviewers:
Florian Schneider

CC:
reviews_dartlang.org

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

Visibility:
Public.

More Reviews

Description

Allow bound check elimination to eliminate checks when both array length and index boundaries are expressed through the same symbol. For example: var list = new List(n); for (var i = 0; i < n; i++) list[i]; R=fschneider@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=14293

Patch Set 1 #

Total comments: 18

Patch Set 2 : address Florian's comments #

Created: 8 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+251 lines, -94 lines)			Patch
M	runtime/vm/flow_graph_builder.cc	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	runtime/vm/flow_graph_optimizer.h	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	runtime/vm/flow_graph_optimizer.cc	View	1	8 chunks	+148 lines, -44 lines	0 comments	Download
M	runtime/vm/hash_map.h	View	1	8 chunks	+61 lines, -25 lines	0 comments	Download
M	runtime/vm/hash_map_test.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	runtime/vm/intermediate_language.h	View		5 chunks	+13 lines, -3 lines	0 comments	Download
M	runtime/vm/intermediate_language.cc	View		4 chunks	+22 lines, -18 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Florian Schneider

lgtm https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimizer.cc File runtime/vm/flow_graph_optimizer.cc (right): https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimizer.cc#newcode1710 runtime/vm/flow_graph_optimizer.cc:1710: static inline intptr_t IsKeyEqual(const ArrayLengthData& kv, Key key) ...

8 years, 1 month ago (2012-10-30 15:31:39 UTC) #2

Vyacheslav Egorov (Google)

8 years, 1 month ago (2012-10-30 16:22:35 UTC) #3

Thanks! Landing.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1710: static inline intptr_t IsKeyEqual(const
ArrayLengthData& kv, Key key) {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> bool IsKeyEqual?

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1953: if (use->instruction()->IsBranch()) {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Please make sure that both cases are triggered by our tests.

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1978: } else {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Please make sure that both cases are triggered by our tests.
> 

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1978: } else {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Please make sure that both cases are triggered by our tests.
> 

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1988: /* length_load->InsertAfter(prev); */
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1988: /* length_load->InsertAfter(prev); */
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/flow_graph_optimiz...
runtime/vm/flow_graph_optimizer.cc:1998: void
RangeAnalysis::ConstrainValueAfterCheckArrayBound(Definition* defn,
On 2012/10/30 15:31:39, Florian Schneider wrote:
> Maybe break the line after (.

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/hash_map.h
File runtime/vm/hash_map.h (right):

https://codereview.chromium.org/11273111/diff/1/runtime/vm/hash_map.h#newcode79
runtime/vm/hash_map.h:79: template <typename T>
On 2012/10/30 15:31:39, Florian Schneider wrote:
> s/T/KeyValueTrait/

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/hash_map.h#newcode205
runtime/vm/hash_map.h:205: static Value ValueOf(Pair p) {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> for naming consistency use either "Pair kv" or "Pair p" in all places.

Done.

https://codereview.chromium.org/11273111/diff/1/runtime/vm/hash_map.h#newcode213
runtime/vm/hash_map.h:213: static inline intptr_t IsKeyEqual(Pair kv, Key key) {
On 2012/10/30 15:31:39, Florian Schneider wrote:
> bool IsKeyEqual?

Done.

Expand Messages | Collapse Messages