Fix scheduler to correctly schedule nested diamonds.

src/compiler/scheduler.cc

Index: src/compiler/scheduler.cc
diff --git a/src/compiler/scheduler.cc b/src/compiler/scheduler.cc
index 58878a0776bfeb483cb68811df0066db32bbf586..a72f91690fa88aec2264087c3acec95c41d28482 100644
--- a/src/compiler/scheduler.cc
+++ b/src/compiler/scheduler.cc
@@ -645,13 +645,18 @@ bool Scheduler::ConnectFloatingControl() {
     bool one_placed = false;
     for (int j = static_cast<int>(block->nodes_.size()) - 1; j >= 0; j--) {
       Node* node = block->nodes_[j];
-      SchedulerData* data = GetData(node);
-      if (data->is_floating_control_ && !data->is_connected_control_ &&
-          !one_placed) {
-        Trace("  Floating control #%d:%s was scheduled in B%d\n", node->id(),
-              node->op()->mnemonic(), block->id());
-        ConnectFloatingControlSubgraph(block, node);
-        one_placed = true;
+      if (!one_placed && (NodeProperties::IsControl(node) ||
+                          OperatorProperties::HasControlInput(node->op()))) {
+        Node* ctl = NodeProperties::IsControl(node)
+                        ? node
+                        : NodeProperties::GetControlInput(node);
+        SchedulerData* data = GetData(ctl);
+        if (data->is_floating_control_ && !data->is_connected_control_) {
+          Trace("  Floating control #%d:%s was scheduled in B%d\n", ctl->id(),
+                ctl->op()->mnemonic(), block->id());
+          ConnectFloatingControlSubgraph(block, ctl);
+          one_placed = true;

[titzer, 2014/09/25 12:19:24]

So this basically matches the first node that is either a floating control node
or has a floating control node as a control dependency.

This is not quite correct because if there is more than one such non-control
node that depends on a floating control node, then they could in fact be pushed
down to different blocks. Placing the merge in one or the other of the blocks
wouldn't dominate the remaining. E.g. suppose there are two phis hanging off the
merge of a floating diamond. Then the two phis can be scheduled independently of
the merge. The merge definitely dominates the phis. That's where it needs to be
placed.

Now that I look at this loop again, you can just return from this method after
the first one is placed (no need to continue processing the block). That will
simplify this condition a bit.

Actually, I am not even sure if we need to return anything from this method now.

[sigurds, 2014/09/26 11:35:24]

This is a valid point, I added a unit test to expose the faulty behaviour in my
patch:

cctest/test-scheduler/PhisPushedDownToDifferentBranches

+        }
       }
     }
   }