From 00d70d63e6975b0663c50c7c619111152ea1cb1c Mon Sep 17 00:00:00 2001 From: titzer Date: Tue, 17 Feb 2015 03:52:32 -0800 Subject: [PATCH] [turbofan] Fix control reducer for dead loops. Note OSR special case. Also improved robustness of OSR tests. R=mstarzinger@chromium.org BUG= Review URL: https://codereview.chromium.org/920573004 Cr-Commit-Position: refs/heads/master@{#26686} --- src/compiler/control-reducer.cc | 20 ++++----- src/compiler/control-reducer.h | 7 +-- src/compiler/osr.cc | 10 +++++ test/cctest/compiler/test-control-reducer.cc | 6 +-- test/cctest/compiler/test-osr.cc | 45 +++++++++++--------- 5 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/compiler/control-reducer.cc b/src/compiler/control-reducer.cc index 7f97254b37..df302dd8ed 100644 --- a/src/compiler/control-reducer.cc +++ b/src/compiler/control-reducer.cc @@ -384,7 +384,8 @@ class ControlReducerImpl { // Reducer implementation: perform reductions on a node. //=========================================================================== Node* ReduceNode(Node* node) { - if (node->op()->ControlInputCount() == 1) { + if (node->op()->ControlInputCount() == 1 || + node->opcode() == IrOpcode::kLoop) { // If a node has only one control input and it is dead, replace with dead. Node* control = NodeProperties::GetControlInput(node); if (control->opcode() == IrOpcode::kDead) { @@ -611,6 +612,14 @@ void ControlReducer::TrimGraph(Zone* zone, JSGraph* jsgraph) { } +Node* ControlReducer::ReduceMerge(JSGraph* jsgraph, + CommonOperatorBuilder* common, Node* node) { + Zone zone; + ControlReducerImpl impl(&zone, jsgraph, common); + return impl.ReduceMerge(node); +} + + Node* ControlReducer::ReducePhiForTesting(JSGraph* jsgraph, CommonOperatorBuilder* common, Node* node) { @@ -620,15 +629,6 @@ Node* ControlReducer::ReducePhiForTesting(JSGraph* jsgraph, } -Node* ControlReducer::ReduceMergeForTesting(JSGraph* jsgraph, - CommonOperatorBuilder* common, - Node* node) { - Zone zone; - ControlReducerImpl impl(&zone, jsgraph, common); - return impl.ReduceMerge(node); -} - - Node* ControlReducer::ReduceIfNodeForTesting(JSGraph* jsgraph, CommonOperatorBuilder* common, Node* node) { diff --git a/src/compiler/control-reducer.h b/src/compiler/control-reducer.h index 1bed84a5cb..bcbd80e916 100644 --- a/src/compiler/control-reducer.h +++ b/src/compiler/control-reducer.h @@ -28,15 +28,16 @@ class ControlReducer { // 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); + // Testing interface. static Node* ReducePhiForTesting(JSGraph* graph, CommonOperatorBuilder* builder, Node* node); static Node* ReduceIfNodeForTesting(JSGraph* graph, CommonOperatorBuilder* builder, Node* node); - static Node* ReduceMergeForTesting(JSGraph* graph, - CommonOperatorBuilder* builder, - Node* node); }; } } diff --git a/src/compiler/osr.cc b/src/compiler/osr.cc index b0592c9d4c..e05e75b927 100644 --- a/src/compiler/osr.cc +++ b/src/compiler/osr.cc @@ -218,6 +218,16 @@ bool OsrHelper::Deconstruct(JSGraph* jsgraph, CommonOperatorBuilder* common, // and run the control reducer to clean up the graph. osr_normal_entry->ReplaceUses(dead); osr_loop_entry->ReplaceUses(graph->start()); + + // Normally the control reducer removes loops whose first input is dead, + // but we need to avoid that because the osr_loop is reachable through + // the second input, so reduce it and its phis manually. + osr_loop->ReplaceInput(0, dead); + Node* node = ControlReducer::ReduceMerge(jsgraph, common, osr_loop); + if (node != osr_loop) osr_loop->ReplaceUses(node); + + // Run the normal control reduction, which naturally trims away the dead + // parts of the graph. ControlReducer::ReduceGraph(tmp_zone, jsgraph, common); return true; diff --git a/test/cctest/compiler/test-control-reducer.cc b/test/cctest/compiler/test-control-reducer.cc index 84b6b4bea5..5b71df4d22 100644 --- a/test/cctest/compiler/test-control-reducer.cc +++ b/test/cctest/compiler/test-control-reducer.cc @@ -164,8 +164,7 @@ class ControlReducerTester : HandleAndZoneScope { } void ReduceMerge(Node* expect, Node* merge) { - Node* result = - ControlReducer::ReduceMergeForTesting(&jsgraph, &common, merge); + Node* result = ControlReducer::ReduceMerge(&jsgraph, &common, merge); CHECK_EQ(expect, result); } @@ -874,8 +873,7 @@ TEST(CMergeReduce_exhaustive_4) { if (!selector.is_selected(i)) merge->ReplaceInput(i, R.dead); } - Node* result = - ControlReducer::ReduceMergeForTesting(&R.jsgraph, &R.common, merge); + Node* result = ControlReducer::ReduceMerge(&R.jsgraph, &R.common, merge); int count = selector.count; if (count == 0) { diff --git a/test/cctest/compiler/test-osr.cc b/test/cctest/compiler/test-osr.cc index 57b4ab1a5e..c23fde6a00 100644 --- a/test/cctest/compiler/test-osr.cc +++ b/test/cctest/compiler/test-osr.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "src/codegen.h" +#include "src/compiler/all-nodes.h" #include "src/compiler/common-operator.h" #include "src/compiler/diamond.h" #include "src/compiler/graph.h" @@ -112,6 +113,17 @@ class OsrDeconstructorTester : public HandleAndZoneScope { Node* NewOsrLoop(int num_backedges, Node* entry = NULL) { return NewLoop(true, num_backedges, entry); } + + void DeconstructOsr() { + OsrHelper helper(0, 0); + helper.Deconstruct(&jsgraph, &common, main_zone()); + AllNodes nodes(main_zone(), &graph); + // Should be edited out. + CHECK(!nodes.IsLive(osr_normal_entry)); + CHECK(!nodes.IsLive(osr_loop_entry)); + // No dangling nodes should be left over. + CHECK_EQ(0u, nodes.gray.size()); + } }; @@ -122,8 +134,7 @@ TEST(Deconstruct_osr0) { T.graph.SetEnd(loop); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, loop); } @@ -139,8 +150,7 @@ TEST(Deconstruct_osr1) { Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, loop); T.graph.SetEnd(ret); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, loop); CheckInputs(osr_phi, T.osr_values[0], T.jsgraph.ZeroConstant(), loop); @@ -160,18 +170,18 @@ TEST(Deconstruct_osr_remove_prologue) { Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, loop); T.graph.SetEnd(ret); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, loop); CheckInputs(osr_phi, T.osr_values[0], T.jsgraph.ZeroConstant(), loop); CheckInputs(ret, osr_phi, T.start, loop); // The control before the loop should have been removed. - CHECK(d.branch->IsDead()); - CHECK(d.if_true->IsDead()); - CHECK(d.if_false->IsDead()); - CHECK(d.merge->IsDead()); + AllNodes nodes(T.main_zone(), &T.graph); + CHECK(!nodes.IsLive(d.branch)); + CHECK(!nodes.IsLive(d.if_true)); + CHECK(!nodes.IsLive(d.if_false)); + CHECK(!nodes.IsLive(d.merge)); } @@ -191,8 +201,7 @@ TEST(Deconstruct_osr_with_body1) { Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, if_false); T.graph.SetEnd(ret); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, if_true); CheckInputs(branch, T.p0, loop); @@ -225,8 +234,7 @@ TEST(Deconstruct_osr_with_body2) { Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, merge); T.graph.SetEnd(ret); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, if_true2); CheckInputs(branch1, T.p0, loop); @@ -265,8 +273,7 @@ TEST(Deconstruct_osr_with_body3) { Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, if_false2); T.graph.SetEnd(ret); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); CheckInputs(loop, T.start, if_false1, if_true2); CheckInputs(branch1, T.p0, loop); @@ -342,8 +349,7 @@ TEST(Deconstruct_osr_nested1) { Node* end = T.graph.NewNode(T.common.End(), ret); T.graph.SetEnd(end); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); // Check structure of deconstructed graph. // Check inner OSR loop is directly connected to start. @@ -410,8 +416,7 @@ TEST(Deconstruct_osr_nested2) { Node* end = T.graph.NewNode(T.common.End(), ret); T.graph.SetEnd(end); - OsrHelper helper(0, 0); - helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone()); + T.DeconstructOsr(); // Check structure of deconstructed graph. // Check inner OSR loop is directly connected to start.