[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

When trying to clone a branch, the ControlFlowOptimizer gave up as soon as it found a Phi/EffectPhi node that could not be placed directly below the IfTrue or IfFalse control paths.

Moving the step in the EffectControlLinearizer phase, after the first schedule, works around the problem by looking at the successor blocks.

BUG=

Committed: https://crrev.com/60c95d85ab3468df93338c35f1841dc2ed3dfd1f
Cr-Commit-Position: refs/heads/master@{#37687}

[epertoso, 2016-07-11 12:17:59]

Patchset #1 (id:1) has been deleted

[epertoso, 2016-07-11 12:18:05]

The CQ bit was checked by epertoso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 12:18:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 12:20:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 12:20:47]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)

[epertoso, 2016-07-11 12:40:10]

The CQ bit was checked by epertoso@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 12:40:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[epertoso, 2016-07-11 12:57:23]

Description was changed from

==========
[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

When trying to clone a branch, the ControlFlowOptimizer gave up as soon as it
found a Phi/EffectPhi node that could not be placed directly below the IfTrue or
IfFalse control path.

Moving the step in the EffectControlLinearizer phase, after the first schedule,
works around the problem by looking at the successor blocks.

BUG=
==========

to

==========
[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

When trying to clone a branch, the ControlFlowOptimizer gave up as soon as it
found a Phi/EffectPhi node that could not be placed directly below the IfTrue or
IfFalse control paths.

Moving the step in the EffectControlLinearizer phase, after the first schedule,
works around the problem by looking at the successor blocks.

BUG=
==========

[epertoso, 2016-07-11 13:02:21]

epertoso@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org

[epertoso, 2016-07-11 13:03:19]

PTAL: this feel hackish to me, but it works.

Running the benchmarks locally, it doesn't seem to make a huge difference
performance-wise.

[commit-bot: I haz the power, 2016-07-11 13:05:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 13:05:30]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-12 03:16:15]

Still a bit rough around the edges, but looking good in general. First round of
comments.

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:45: BasicBlock* from;
How about using the rpo numbers as keys instead of the pointers? That would make
the comparator below easier.

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:79: block_effects->find({predecessor,
block});
This kind of uniform initialization is not supported by Chromium C++ Status
AFAIK.
Maybe you can encapsulate the lookup in some helper method on the Map class.

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:99: return;  // We already wired
stuff.
Nit: Add { } around the return

[epertoso, 2016-07-12 10:14:44]

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:45: BasicBlock* from;
On 2016/07/12 at 03:16:14, Benedikt Meurer wrote:
> How about using the rpo numbers as keys instead of the pointers? That would
make the comparator below easier.

Done.

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:79: block_effects->find({predecessor,
block});
On 2016/07/12 at 03:16:14, Benedikt Meurer wrote:
> This kind of uniform initialization is not supported by Chromium C++ Status
AFAIK.
> Maybe you can encapsulate the lookup in some helper method on the Map class.

Explicitly using the constructor now.

https://codereview.chromium.org/2139593002/diff/40001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:99: return;  // We already wired
stuff.
On 2016/07/12 at 03:16:15, Benedikt Meurer wrote:
> Nit: Add { } around the return

Done.

[Benedikt Meurer, 2016-07-12 12:46:16]

bmeurer@chromium.org changed required reviewers:
+ jarin@chromium.org

[Benedikt Meurer, 2016-07-12 12:46:16]

LGTM from my side.

[Jarin, 2016-07-12 12:58:53]

lgtm

https://codereview.chromium.org/2139593002/diff/60001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2139593002/diff/60001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:446: } break;
The break should go into the braces. 

In fact, could not you have special case for kBranch that does 

ProcessNode(block->control_input(), &frame_state, &effect, &control);
TryCloneBranch(block->control_input(), block, graph(), common(),
               &block_effects);
break;

[epertoso, 2016-07-12 14:58:51]

Patchset #4 (id:80001) has been deleted

[epertoso, 2016-07-12 14:59:01]

https://codereview.chromium.org/2139593002/diff/60001/src/compiler/effect-con...
File src/compiler/effect-control-linearizer.cc (right):

https://codereview.chromium.org/2139593002/diff/60001/src/compiler/effect-con...
src/compiler/effect-control-linearizer.cc:446: } break;
On 2016/07/12 at 12:58:53, Jarin wrote:
> The break should go into the braces. 
> 
> In fact, could not you have special case for kBranch that does 
> 
> ProcessNode(block->control_input(), &frame_state, &effect, &control);
> TryCloneBranch(block->control_input(), block, graph(), common(),
>                &block_effects);
> break;

Done.

[epertoso, 2016-07-12 14:59:04]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2016-07-12 14:59:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2139593002/#ps100001
(title: "Update.")

[commit-bot: I haz the power, 2016-07-12 14:59:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 15:22:20]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-07-12 15:23:44]

Description was changed from

==========
[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

When trying to clone a branch, the ControlFlowOptimizer gave up as soon as it
found a Phi/EffectPhi node that could not be placed directly below the IfTrue or
IfFalse control paths.

Moving the step in the EffectControlLinearizer phase, after the first schedule,
works around the problem by looking at the successor blocks.

BUG=
==========

to

==========
[turbofan] Move TryCloneBranch in the EffectControlLinearizer pass.

When trying to clone a branch, the ControlFlowOptimizer gave up as soon as it
found a Phi/EffectPhi node that could not be placed directly below the IfTrue or
IfFalse control paths.

Moving the step in the EffectControlLinearizer phase, after the first schedule,
works around the problem by looking at the successor blocks.

BUG=

Committed: https://crrev.com/60c95d85ab3468df93338c35f1841dc2ed3dfd1f
Cr-Commit-Position: refs/heads/master@{#37687}
==========

[commit-bot: I haz the power, 2016-07-12 15:23:46]

Patchset 4 (id:??) landed as
https://crrev.com/60c95d85ab3468df93338c35f1841dc2ed3dfd1f
Cr-Commit-Position: refs/heads/master@{#37687}