From 2b59959c71e3e0cc7ce7398465153faf45648ada Mon Sep 17 00:00:00 2001 From: titzer Date: Wed, 8 Apr 2015 04:13:43 -0700 Subject: [PATCH] [turbofan] Match selects in control reducer (configurable). R=bmeurer@chromium.org BUG= Review URL: https://codereview.chromium.org/1061303002 Cr-Commit-Position: refs/heads/master@{#27658} --- src/compiler/control-reducer.cc | 60 +++++++++++---- src/compiler/control-reducer.h | 5 +- src/compiler/node-matchers.h | 14 ++++ src/compiler/pipeline.cc | 17 +++-- .../compiler/control-reducer-unittest.cc | 75 ++++++++++++++++++- 5 files changed, 144 insertions(+), 27 deletions(-) diff --git a/src/compiler/control-reducer.cc b/src/compiler/control-reducer.cc index 08182703b4..9856eee695 100644 --- a/src/compiler/control-reducer.cc +++ b/src/compiler/control-reducer.cc @@ -56,7 +56,8 @@ class ControlReducerImpl { common_(common), state_(jsgraph->graph()->NodeCount(), kUnvisited, zone_), stack_(zone_), - revisit_(zone_) {} + revisit_(zone_), + max_phis_for_select_(0) {} Zone* zone_; JSGraph* jsgraph_; @@ -64,6 +65,7 @@ class ControlReducerImpl { ZoneVector state_; ZoneDeque stack_; ZoneDeque revisit_; + int max_phis_for_select_; void Reduce() { Push(graph()->end()); @@ -536,7 +538,7 @@ class ControlReducerImpl { if (live == 0) return dead(); // no remaining inputs. // Gather phis and effect phis to be edited. - ZoneVector phis(zone_); + NodeVector phis(zone_); for (Node* const use : node->uses()) { if (NodeProperties::IsPhi(use)) phis.push_back(use); } @@ -564,25 +566,49 @@ class ControlReducerImpl { DCHECK_EQ(live, node->InputCount()); - // Check if it's an unused diamond. - if (live == 2 && phis.empty()) { + // Try to remove dead diamonds or introduce selects. + if (live == 2 && CheckPhisForSelect(phis)) { DiamondMatcher matcher(node); if (matcher.Matched() && matcher.IfProjectionsAreOwned()) { - // It's a dead diamond, i.e. neither the IfTrue nor the IfFalse nodes - // have uses except for the Merge and the Merge has no Phi or - // EffectPhi uses, so replace the Merge with the control input of the - // diamond. - TRACE(" DeadDiamond: #%d:Branch #%d:IfTrue #%d:IfFalse\n", - matcher.Branch()->id(), matcher.IfTrue()->id(), - matcher.IfFalse()->id()); - // TODO(turbofan): replace single-phi diamonds with selects. - return NodeProperties::GetControlInput(matcher.Branch()); + // Dead diamond, i.e. neither the IfTrue nor the IfFalse nodes + // have uses except for the Merge. Remove the branch if there + // are no phis or replace phis with selects. + Node* control = NodeProperties::GetControlInput(matcher.Branch()); + if (phis.size() == 0) { + // No phis. Remove the branch altogether. + TRACE(" DeadDiamond: #%d:Branch #%d:IfTrue #%d:IfFalse\n", + matcher.Branch()->id(), matcher.IfTrue()->id(), + matcher.IfFalse()->id()); + return control; + } else { + // A small number of phis. Replace with selects. + Node* cond = matcher.Branch()->InputAt(0); + for (Node* phi : phis) { + Node* select = graph()->NewNode( + common_->Select(OpParameter(phi), + BranchHintOf(matcher.Branch()->op())), + cond, matcher.TrueInputOf(phi), matcher.FalseInputOf(phi)); + TRACE(" MatchSelect: #%d:Branch #%d:IfTrue #%d:IfFalse -> #%d\n", + matcher.Branch()->id(), matcher.IfTrue()->id(), + matcher.IfFalse()->id(), select->id()); + ReplaceNode(phi, select); + } + return control; + } } } return node; } + bool CheckPhisForSelect(const NodeVector& phis) { + if (phis.size() > static_cast(max_phis_for_select_)) return false; + for (Node* phi : phis) { + if (phi->opcode() != IrOpcode::kPhi) return false; // no EffectPhis. + } + return true; + } + // Reduce if projections if the branch has a constant input. Node* ReduceIfProjection(Node* node, Decision decision) { Node* branch = node->InputAt(0); @@ -638,8 +664,10 @@ class ControlReducerImpl { void ControlReducer::ReduceGraph(Zone* zone, JSGraph* jsgraph, - CommonOperatorBuilder* common) { + CommonOperatorBuilder* common, + int max_phis_for_select) { ControlReducerImpl impl(zone, jsgraph, common); + impl.max_phis_for_select_ = max_phis_for_select; impl.Reduce(); } @@ -651,9 +679,11 @@ void ControlReducer::TrimGraph(Zone* zone, JSGraph* jsgraph) { Node* ControlReducer::ReduceMerge(JSGraph* jsgraph, - CommonOperatorBuilder* common, Node* node) { + CommonOperatorBuilder* common, Node* node, + int max_phis_for_select) { Zone zone; ControlReducerImpl impl(&zone, jsgraph, common); + impl.max_phis_for_select_ = max_phis_for_select; return impl.ReduceMerge(node); } diff --git a/src/compiler/control-reducer.h b/src/compiler/control-reducer.h index bcbd80e916..1e29e01e85 100644 --- a/src/compiler/control-reducer.h +++ b/src/compiler/control-reducer.h @@ -23,14 +23,15 @@ class ControlReducer { public: // Perform branch folding and dead code elimination on the graph. static void ReduceGraph(Zone* zone, JSGraph* graph, - CommonOperatorBuilder* builder); + CommonOperatorBuilder* builder, + int max_phis_for_select = 0); // Trim nodes in the graph that are not reachable from end. static void TrimGraph(Zone* zone, JSGraph* graph); // Reduces a single merge node and attached phis. static Node* ReduceMerge(JSGraph* graph, CommonOperatorBuilder* builder, - Node* node); + Node* node, int max_phis_for_select = 0); // Testing interface. static Node* ReducePhiForTesting(JSGraph* graph, diff --git a/src/compiler/node-matchers.h b/src/compiler/node-matchers.h index 627829f4b5..6beca7c2c8 100644 --- a/src/compiler/node-matchers.h +++ b/src/compiler/node-matchers.h @@ -570,6 +570,20 @@ struct DiamondMatcher : public NodeMatcher { Node* IfFalse() const { return if_false_; } Node* Merge() const { return node(); } + Node* TrueInputOf(Node* phi) const { + DCHECK(IrOpcode::IsPhiOpcode(phi->opcode())); + DCHECK_EQ(3, phi->InputCount()); + DCHECK_EQ(Merge(), phi->InputAt(2)); + return phi->InputAt(if_true_ == Merge()->InputAt(0) ? 0 : 1); + } + + Node* FalseInputOf(Node* phi) const { + DCHECK(IrOpcode::IsPhiOpcode(phi->opcode())); + DCHECK_EQ(3, phi->InputCount()); + DCHECK_EQ(Merge(), phi->InputAt(2)); + return phi->InputAt(if_true_ == Merge()->InputAt(0) ? 1 : 0); + } + private: Node* branch_; Node* if_true_; diff --git a/src/compiler/pipeline.cc b/src/compiler/pipeline.cc index b1d7fda9a3..7b1f61f30e 100644 --- a/src/compiler/pipeline.cc +++ b/src/compiler/pipeline.cc @@ -593,22 +593,23 @@ struct ChangeLoweringPhase { }; -struct ControlReductionPhase { +struct EarlyControlReductionPhase { + static const char* phase_name() { return "early control reduction"; } void Run(PipelineData* data, Zone* temp_zone) { SourcePositionTable::Scope pos(data->source_positions(), SourcePosition::Unknown()); - ControlReducer::ReduceGraph(temp_zone, data->jsgraph(), data->common()); + ControlReducer::ReduceGraph(temp_zone, data->jsgraph(), data->common(), 1); } }; -struct EarlyControlReductionPhase : ControlReductionPhase { - static const char* phase_name() { return "early control reduction"; } -}; - - -struct LateControlReductionPhase : ControlReductionPhase { +struct LateControlReductionPhase { static const char* phase_name() { return "late control reduction"; } + void Run(PipelineData* data, Zone* temp_zone) { + SourcePositionTable::Scope pos(data->source_positions(), + SourcePosition::Unknown()); + ControlReducer::ReduceGraph(temp_zone, data->jsgraph(), data->common(), 0); + } }; diff --git a/test/unittests/compiler/control-reducer-unittest.cc b/test/unittests/compiler/control-reducer-unittest.cc index 76f25580bd..02ff8ffd7c 100644 --- a/test/unittests/compiler/control-reducer-unittest.cc +++ b/test/unittests/compiler/control-reducer-unittest.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "src/compiler/control-reducer.h" +#include "src/compiler/diamond.h" #include "src/compiler/graph-visualizer.h" #include "src/compiler/js-graph.h" #include "src/compiler/js-operator.h" @@ -34,13 +35,14 @@ class ControlReducerTest : public TypedGraphTest { JSOperatorBuilder javascript_; JSGraph jsgraph_; - void ReduceGraph() { + void ReduceGraph(int max_phis_for_select = 0) { if (FLAG_trace_turbo_graph) { OFStream os(stdout); os << "-- Graph before control reduction" << std::endl; os << AsRPO(*graph()); } - ControlReducer::ReduceGraph(zone(), jsgraph(), common()); + ControlReducer::ReduceGraph(zone(), jsgraph(), common(), + max_phis_for_select); if (FLAG_trace_turbo_graph) { OFStream os(stdout); os << "-- Graph after control reduction" << std::endl; @@ -279,6 +281,75 @@ TEST_F(ControlReducerTest, RangeAsInputToBranch_true2) { } +TEST_F(ControlReducerTest, SelectPhi) { + Node* p0 = Parameter(0); + const MachineType kType = kMachInt32; + Diamond d(graph(), common(), p0); + Node* phi = + d.Phi(kType, jsgraph()->Int32Constant(1), jsgraph()->Int32Constant(2)); + + Node* ret = + graph()->NewNode(common()->Return(), phi, graph()->start(), d.merge); + graph()->end()->ReplaceInput(0, ret); + + ReduceGraph(1); + + // Phi should be replaced with a select. + EXPECT_THAT(graph()->end(), + IsEnd(IsReturn( + IsSelect(kType, p0, IsInt32Constant(1), IsInt32Constant(2)), + graph()->start(), graph()->start()))); +} + + +TEST_F(ControlReducerTest, SelectPhis_fail) { + Node* p0 = Parameter(0); + const MachineType kType = kMachInt32; + Diamond d(graph(), common(), p0); + Node* phi = + d.Phi(kType, jsgraph()->Int32Constant(1), jsgraph()->Int32Constant(2)); + Node* phi2 = + d.Phi(kType, jsgraph()->Int32Constant(11), jsgraph()->Int32Constant(22)); + USE(phi2); + Node* ret = + graph()->NewNode(common()->Return(), phi, graph()->start(), d.merge); + graph()->end()->ReplaceInput(0, ret); + + ReduceGraph(1); + + // Diamond should not be replaced with a select (too many phis). + EXPECT_THAT(ret, IsReturn(phi, graph()->start(), d.merge)); + EXPECT_THAT(graph()->end(), IsEnd(ret)); +} + + +TEST_F(ControlReducerTest, SelectTwoPhis) { + Node* p0 = Parameter(0); + const MachineType kType = kMachInt32; + Diamond d(graph(), common(), p0); + Node* phi1 = + d.Phi(kType, jsgraph()->Int32Constant(1), jsgraph()->Int32Constant(2)); + Node* phi2 = + d.Phi(kType, jsgraph()->Int32Constant(2), jsgraph()->Int32Constant(3)); + MachineOperatorBuilder machine(zone()); + Node* add = graph()->NewNode(machine.Int32Add(), phi1, phi2); + Node* ret = + graph()->NewNode(common()->Return(), add, graph()->start(), d.merge); + graph()->end()->ReplaceInput(0, ret); + + ReduceGraph(2); + + // Phis should be replaced with two selects. + EXPECT_THAT( + ret, + IsReturn(IsInt32Add( + IsSelect(kType, p0, IsInt32Constant(1), IsInt32Constant(2)), + IsSelect(kType, p0, IsInt32Constant(2), IsInt32Constant(3))), + graph()->start(), graph()->start())); + EXPECT_THAT(graph()->end(), IsEnd(ret)); +} + + } // namespace compiler } // namespace internal } // namespace v8