From cf08e8d70ff0fa35dd315ddc430b743bb04c89cf Mon Sep 17 00:00:00 2001 From: "bmeurer@chromium.org" Date: Mon, 10 Nov 2014 10:29:37 +0000 Subject: [PATCH] [turbofan] Fix select lowering. Select lowering must not merge Select nodes that depend on each other, because the resulting graph is not schedulable. TEST=unittests R=jarin@chromium.org Review URL: https://codereview.chromium.org/717473002 Cr-Commit-Position: refs/heads/master@{#25236} git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@25236 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/compiler/node.h | 2 + src/compiler/select-lowering.cc | 54 +++++++++++++++---- src/compiler/select-lowering.h | 6 ++- .../compiler/select-lowering-unittest.cc | 14 ++++- 4 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/compiler/node.h b/src/compiler/node.h index 3a5afd25da..8a442cedbc 100644 --- a/src/compiler/node.h +++ b/src/compiler/node.h @@ -68,6 +68,8 @@ typedef std::set, zone_allocator > NodeSet; typedef NodeSet::iterator NodeSetIter; typedef NodeSet::reverse_iterator NodeSetRIter; +typedef ZoneDeque NodeDeque; + typedef ZoneVector NodeVector; typedef NodeVector::iterator NodeVectorIter; typedef NodeVector::const_iterator NodeVectorConstIter; diff --git a/src/compiler/select-lowering.cc b/src/compiler/select-lowering.cc index 2e51d72024..44d040df04 100644 --- a/src/compiler/select-lowering.cc +++ b/src/compiler/select-lowering.cc @@ -26,26 +26,58 @@ Reduction SelectLowering::Reduce(Node* node) { if (node->opcode() != IrOpcode::kSelect) return NoChange(); SelectParameters const p = SelectParametersOf(node->op()); - Node* const cond = node->InputAt(0); + Node* cond = node->InputAt(0); + Node* vthen = node->InputAt(1); + Node* velse = node->InputAt(2); + Node* merge = nullptr; // Check if we already have a diamond for this condition. - auto i = merges_.find(cond); - if (i == merges_.end()) { - // Create a new diamond for this condition and remember its merge node. - Diamond d(graph(), common(), cond, p.hint()); - i = merges_.insert(std::make_pair(cond, d.merge)).first; - } + auto range = merges_.equal_range(cond); + for (auto i = range.first;; ++i) { + if (i == range.second) { + // Create a new diamond for this condition and remember its merge node. + Diamond d(graph(), common(), cond, p.hint()); + merges_.insert(std::make_pair(cond, d.merge)); + merge = d.merge; + break; + } - DCHECK_EQ(cond, i->first); + // If the diamond is reachable from the Select, merging them would result in + // an unschedulable graph, so we cannot reuse the diamond in that case. + merge = i->second; + if (!ReachableFrom(merge, node)) { + break; + } + } // Create a Phi hanging off the previously determined merge. node->set_op(common()->Phi(p.type(), 2)); - node->ReplaceInput(0, node->InputAt(1)); - node->ReplaceInput(1, node->InputAt(2)); - node->ReplaceInput(2, i->second); + node->ReplaceInput(0, vthen); + node->ReplaceInput(1, velse); + node->ReplaceInput(2, merge); return Changed(node); } + +bool SelectLowering::ReachableFrom(Node* const sink, Node* const source) { + // TODO(turbofan): This is probably horribly expensive, and it should be moved + // into node.h or somewhere else?! + Zone zone(graph()->zone()->isolate()); + std::queue pending((NodeDeque(&zone))); + BoolVector visited(graph()->NodeCount(), false, &zone); + pending.push(source); + while (!pending.empty()) { + Node* current = pending.front(); + if (current == sink) return true; + pending.pop(); + visited[current->id()] = true; + for (auto input : current->inputs()) { + if (!visited[input->id()]) pending.push(input); + } + } + return false; +} + } // namespace compiler } // namespace internal } // namespace v8 diff --git a/src/compiler/select-lowering.h b/src/compiler/select-lowering.h index ae22cade84..05ea0e0c75 100644 --- a/src/compiler/select-lowering.h +++ b/src/compiler/select-lowering.h @@ -28,8 +28,10 @@ class SelectLowering FINAL : public Reducer { Reduction Reduce(Node* node) OVERRIDE; private: - typedef std::map, - zone_allocator>> Merges; + typedef std::multimap, + zone_allocator>> Merges; + + bool ReachableFrom(Node* const sink, Node* const source); CommonOperatorBuilder* common() const { return common_; } Graph* graph() const { return graph_; } diff --git a/test/unittests/compiler/select-lowering-unittest.cc b/test/unittests/compiler/select-lowering-unittest.cc index 6dbd7ad73d..51efc83f87 100644 --- a/test/unittests/compiler/select-lowering-unittest.cc +++ b/test/unittests/compiler/select-lowering-unittest.cc @@ -10,6 +10,7 @@ using testing::AllOf; using testing::Capture; using testing::CaptureEq; +using testing::Not; namespace v8 { namespace internal { @@ -33,12 +34,12 @@ TEST_F(SelectLoweringTest, SelectWithSameConditions) { Node* const p2 = Parameter(2); Node* const p3 = Parameter(3); Node* const p4 = Parameter(4); + Node* const s0 = graph()->NewNode(common()->Select(kMachInt32), p0, p1, p2); Capture branch; Capture merge; { - Reduction const r = - Reduce(graph()->NewNode(common()->Select(kMachInt32), p0, p1, p2)); + Reduction const r = Reduce(s0); ASSERT_TRUE(r.Changed()); EXPECT_THAT( r.replacement(), @@ -55,6 +56,15 @@ TEST_F(SelectLoweringTest, SelectWithSameConditions) { ASSERT_TRUE(r.Changed()); EXPECT_THAT(r.replacement(), IsPhi(kMachInt32, p3, p4, CaptureEq(&merge))); } + { + // We must not reuse the diamond if it is reachable from either else/then + // values of the Select, because the resulting graph can not be scheduled. + Reduction const r = + Reduce(graph()->NewNode(common()->Select(kMachInt32), p0, s0, p0)); + ASSERT_TRUE(r.Changed()); + EXPECT_THAT(r.replacement(), + IsPhi(kMachInt32, s0, p0, Not(CaptureEq(&merge)))); + } } } // namespace compiler