[turbofan] optimize hot loop in ResolveControlFlow

[turbofan] optimize hot loop in ResolveControlFlow

R=jarin@chromium.org

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=25149

[dcarney, 2014-10-31 09:59:03]

Patchset #1 (id:1) has been deleted

[dcarney, 2014-10-31 09:59:16]

Patchset #1 (id:20001) has been deleted

[dcarney, 2014-10-31 09:59:21]

Patchset #1 (id:40001) has been deleted

[dcarney, 2014-10-31 09:59:26]

Patchset #1 (id:60001) has been deleted

[dcarney, 2014-11-03 10:08:10]

Patchset #5 (id:160001) has been deleted

[dcarney, 2014-11-03 13:58:35]

dcarney@chromium.org changed reviewers:
+ jarin@chromium.org

[Jarin, 2014-11-04 10:42:09]

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1302: LiveRangeBound* Find(const
LifetimePosition position) {
Could not we use std::lower_bound for this?

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1310: DCHECK(right_index != current_index);
How about DCHECK(right_index > current_index)?

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1316: left_index = right_index;
I think this degenerate case would not appear if you calculated
current_index = left_index + (right_index + 1 - left_index) / 2;
(or if your right_index was exclusive).

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1361: bool SetCurrent(int operand_index) {
Maybe this could return the LiveRangeBound pointer so that there is not this
funny 'current_' state in this class (then it is basically just a cache for the
bound arrays).

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1363: // TODO(dcarney): can this happen at
this point?
Yeah, just DCHECK here that the things that should not happen do not happen.

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.h:354: Zone* zone() const { return zone_; }
Since you already touching this, could you possibly rename to local_zone()?

[dcarney, 2014-11-05 08:59:49]

Patchset #8 (id:240001) has been deleted

[dcarney, 2014-11-05 09:00:06]

Patchset #7 (id:220001) has been deleted

[dcarney, 2014-11-05 09:02:02]

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1302: LiveRangeBound* Find(const
LifetimePosition position) {
On 2014/11/04 10:42:08, jarin wrote:
> Could not we use std::lower_bound for this?

benchmarking says no
i refactored the current code and it drops from 3.5% of the compilation time of
zlib to 3
with lower_bound it's around 4

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1310: DCHECK(right_index != current_index);
On 2014/11/04 10:42:08, jarin wrote:
> How about DCHECK(right_index > current_index)?

Done.

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1316: left_index = right_index;
On 2014/11/04 10:42:09, jarin wrote:
> I think this degenerate case would not appear if you calculated
> current_index = left_index + (right_index + 1 - left_index) / 2;
> (or if your right_index was exclusive).

Done.

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1361: bool SetCurrent(int operand_index) {
On 2014/11/04 10:42:09, jarin wrote:
> Maybe this could return the LiveRangeBound pointer so that there is not this
> funny 'current_' state in this class (then it is basically just a cache for
the
> bound arrays).

Done.

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.cc:1363: // TODO(dcarney): can this happen at
this point?
On 2014/11/04 10:42:09, jarin wrote:
> Yeah, just DCHECK here that the things that should not happen do not happen.

Done.

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/694473002/diff/200001/src/compiler/register-a...
src/compiler/register-allocator.h:354: Zone* zone() const { return zone_; }
On 2014/11/04 10:42:09, jarin wrote:
> Since you already touching this, could you possibly rename to local_zone()?

Done.

[Jarin, 2014-11-05 12:30:18]

lgtm

https://codereview.chromium.org/694473002/diff/260001/src/compiler/register-a...
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/694473002/diff/260001/src/compiler/register-a...
src/compiler/register-allocator.cc:1304: } else {
Thanks, this looks much better!

[dcarney, 2014-11-05 12:46:51]

Committed patchset #7 (id:260001) manually as 25149 (presubmit successful).