[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
This commit is contained in:
bmeurer@chromium.org 2014-11-10 10:29:37 +00:00
parent e140bbcafa
commit cf08e8d70f
4 changed files with 61 additions and 15 deletions

View File

@ -68,6 +68,8 @@ typedef std::set<Node*, std::less<Node*>, zone_allocator<Node*> > NodeSet;
typedef NodeSet::iterator NodeSetIter; typedef NodeSet::iterator NodeSetIter;
typedef NodeSet::reverse_iterator NodeSetRIter; typedef NodeSet::reverse_iterator NodeSetRIter;
typedef ZoneDeque<Node*> NodeDeque;
typedef ZoneVector<Node*> NodeVector; typedef ZoneVector<Node*> NodeVector;
typedef NodeVector::iterator NodeVectorIter; typedef NodeVector::iterator NodeVectorIter;
typedef NodeVector::const_iterator NodeVectorConstIter; typedef NodeVector::const_iterator NodeVectorConstIter;

View File

@ -26,26 +26,58 @@ Reduction SelectLowering::Reduce(Node* node) {
if (node->opcode() != IrOpcode::kSelect) return NoChange(); if (node->opcode() != IrOpcode::kSelect) return NoChange();
SelectParameters const p = SelectParametersOf(node->op()); 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. // Check if we already have a diamond for this condition.
auto i = merges_.find(cond); auto range = merges_.equal_range(cond);
if (i == merges_.end()) { for (auto i = range.first;; ++i) {
// Create a new diamond for this condition and remember its merge node. if (i == range.second) {
Diamond d(graph(), common(), cond, p.hint()); // Create a new diamond for this condition and remember its merge node.
i = merges_.insert(std::make_pair(cond, d.merge)).first; 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. // Create a Phi hanging off the previously determined merge.
node->set_op(common()->Phi(p.type(), 2)); node->set_op(common()->Phi(p.type(), 2));
node->ReplaceInput(0, node->InputAt(1)); node->ReplaceInput(0, vthen);
node->ReplaceInput(1, node->InputAt(2)); node->ReplaceInput(1, velse);
node->ReplaceInput(2, i->second); node->ReplaceInput(2, merge);
return Changed(node); 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<Node*, NodeDeque> 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 compiler
} // namespace internal } // namespace internal
} // namespace v8 } // namespace v8

View File

@ -28,8 +28,10 @@ class SelectLowering FINAL : public Reducer {
Reduction Reduce(Node* node) OVERRIDE; Reduction Reduce(Node* node) OVERRIDE;
private: private:
typedef std::map<Node*, Node*, std::less<Node*>, typedef std::multimap<Node*, Node*, std::less<Node*>,
zone_allocator<std::pair<Node* const, Node*>>> Merges; zone_allocator<std::pair<Node* const, Node*>>> Merges;
bool ReachableFrom(Node* const sink, Node* const source);
CommonOperatorBuilder* common() const { return common_; } CommonOperatorBuilder* common() const { return common_; }
Graph* graph() const { return graph_; } Graph* graph() const { return graph_; }

View File

@ -10,6 +10,7 @@
using testing::AllOf; using testing::AllOf;
using testing::Capture; using testing::Capture;
using testing::CaptureEq; using testing::CaptureEq;
using testing::Not;
namespace v8 { namespace v8 {
namespace internal { namespace internal {
@ -33,12 +34,12 @@ TEST_F(SelectLoweringTest, SelectWithSameConditions) {
Node* const p2 = Parameter(2); Node* const p2 = Parameter(2);
Node* const p3 = Parameter(3); Node* const p3 = Parameter(3);
Node* const p4 = Parameter(4); Node* const p4 = Parameter(4);
Node* const s0 = graph()->NewNode(common()->Select(kMachInt32), p0, p1, p2);
Capture<Node*> branch; Capture<Node*> branch;
Capture<Node*> merge; Capture<Node*> merge;
{ {
Reduction const r = Reduction const r = Reduce(s0);
Reduce(graph()->NewNode(common()->Select(kMachInt32), p0, p1, p2));
ASSERT_TRUE(r.Changed()); ASSERT_TRUE(r.Changed());
EXPECT_THAT( EXPECT_THAT(
r.replacement(), r.replacement(),
@ -55,6 +56,15 @@ TEST_F(SelectLoweringTest, SelectWithSameConditions) {
ASSERT_TRUE(r.Changed()); ASSERT_TRUE(r.Changed());
EXPECT_THAT(r.replacement(), IsPhi(kMachInt32, p3, p4, CaptureEq(&merge))); 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 } // namespace compiler