[turbofan] Introduce node regions for protection from scheduling.

[turbofan] Introduce node regions for protection from scheduling.

This CL re-purposes ValueEffect and Finish as delimiters for regions
that are scheduled atomically (renamed to BeginRegion, FinishRegion).

The BeginRegion node takes and produces an effect. For the uses that do
not care about the placement in the effect chain, it is ok to feed
graph->start() as an effect input.

The FinishRegion takes a value and an effect and produces a value and
an effect. It is important that any value or effect produced inside the
region is not used outside the region. The FinishRegion node is the only
way to smuggle an effect and a value out.

At the moment, this does not support control flow inside the region. Control flow would be hard.

During scheduling we do some sanity check, but the checks are not exhaustive. Here is what we check:
- the effect chain between begin and finish is linear (no splitting,
  single effect input and output).
- any value produced is consumed by the FinishRegion node.
- no control flow outputs.

Committed: https://crrev.com/59c616ccd7f9d707f6129c5f726aa652e7bc5f22
Cr-Commit-Position: refs/heads/master@{#31265}

[Jarin, 2015-10-13 14:09:28]

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

[Jarin, 2015-10-13 14:15:28]

Could you take a look, please?

[Benedikt Meurer, 2015-10-13 19:12:09]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2015-10-13 19:12:09]

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

[Benedikt Meurer, 2015-10-13 19:12:10]

Adding Michi as reviewer for JSTypedLowering.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
src/compiler/common-operator.cc:681: const Operator*
CommonOperatorBuilder::BeginRegion() {
Nit: Move to the CACHED_OP_LIST above.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
src/compiler/common-operator.cc:689: const Operator*
CommonOperatorBuilder::FinishRegion() {
Nit: Move to the CACHED_OP_LIST above.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:80: void Finish() {
Nit: I think this helper should return the Reduction (always Changed(node) for
now) to make it explicit that it does mutate the graph.

Also I'm not convinced that this helper improves the readability of the code
below. I think I'd be fine with this, but Michi should have a look as well.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/scheduler.cc
File src/compiler/scheduler.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/scheduler....
src/compiler/scheduler.cc:1577: void ScheduleRegion(BasicBlock* block, Node*
region_end) {
Awesome! Thanks for making the checking generic.

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-typed-lowering-unittest.cc:1071: effect, control),
This test shouldn't pass, because the effect input to the Allocate should a
BeginRegion, not graph->start. There seems to be something wrong here.

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-typed-lowering-unittest.cc:1111: effect, control),
This test shouldn't pass, because the effect input to the Allocate should a
BeginRegion, not graph->start. There seems to be something wrong here.

[Jarin, 2015-10-14 06:37:21]

Addressed the comments.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
File src/compiler/common-operator.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
src/compiler/common-operator.cc:681: const Operator*
CommonOperatorBuilder::BeginRegion() {
On 2015/10/13 19:12:09, Benedikt Meurer wrote:
> Nit: Move to the CACHED_OP_LIST above.

Done.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/common-ope...
src/compiler/common-operator.cc:689: const Operator*
CommonOperatorBuilder::FinishRegion() {
On 2015/10/13 19:12:09, Benedikt Meurer wrote:
> Nit: Move to the CACHED_OP_LIST above.

Done.

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:80: void Finish() {
On 2015/10/13 19:12:09, Benedikt Meurer wrote:
> Nit: I think this helper should return the Reduction (always Changed(node) for
> now) to make it explicit that it does mutate the graph.
> 
> Also I'm not convinced that this helper improves the readability of the code
> below. I think I'd be fine with this, but Michi should have a look as well.

Ack. I will leave the decision to Michi, I do not have a strong preference here.
For now, I changed the Finish method to take the node (and removed the node from
the constructor).

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
File test/unittests/compiler/js-typed-lowering-unittest.cc (right):

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-typed-lowering-unittest.cc:1071: effect, control),
On 2015/10/13 19:12:10, Benedikt Meurer wrote:
> This test shouldn't pass, because the effect input to the Allocate should a
> BeginRegion, not graph->start. There seems to be something wrong here.

Great catch! Fixed. (Unfortunately, we do not run this test because of the test
in the beginning of this test.)

https://codereview.chromium.org/1399423002/diff/40001/test/unittests/compiler...
test/unittests/compiler/js-typed-lowering-unittest.cc:1111: effect, control),
On 2015/10/13 19:12:10, Benedikt Meurer wrote:
> This test shouldn't pass, because the effect input to the Allocate should a
> BeginRegion, not graph->start. There seems to be something wrong here.

Done.

[Benedikt Meurer, 2015-10-14 10:56:51]

bmeurer@chromium.org changed required reviewers:
- mstarzinger@chromium.org

[Benedikt Meurer, 2015-10-14 10:56:52]

Thanks. LGTM from my side.

[Michael Starzinger, 2015-10-14 13:58:13]

LGTM on just JSTypedLowering with one nit, to unblock. I didn't look at the
rest. I'll take a look at the rest later if you want me to.

https://codereview.chromium.org/1399423002/diff/60001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1399423002/diff/60001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:1272: RelaxControls(node);
nit: As discussed offline, can we move this to just before the
AllocationBuilder::Finish call.

[Jarin, 2015-10-14 14:42:34]

Thanks!

https://codereview.chromium.org/1399423002/diff/60001/src/compiler/js-typed-l...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1399423002/diff/60001/src/compiler/js-typed-l...
src/compiler/js-typed-lowering.cc:1272: RelaxControls(node);
On 2015/10/14 13:58:13, Michael Starzinger wrote:
> nit: As discussed offline, can we move this to just before the
> AllocationBuilder::Finish call.

Done.

[Jarin, 2015-10-14 14:42:56]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2015-10-14 14:42:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1399423002/#ps80001
(title: "Move RelaxControls")

[commit-bot: I haz the power, 2015-10-14 14:43:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1399423002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1399423002/80001

[commit-bot: I haz the power, 2015-10-14 14:53:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-14 14:53:18]

Patchset 5 (id:??) landed as
https://crrev.com/59c616ccd7f9d707f6129c5f726aa652e7bc5f22
Cr-Commit-Position: refs/heads/master@{#31265}