diff --git a/src/hotspot/share/opto/cfgnode.cpp b/src/hotspot/share/opto/cfgnode.cpp index c8deb75b80e37..c8dfd66ca21ad 100644 --- a/src/hotspot/share/opto/cfgnode.cpp +++ b/src/hotspot/share/opto/cfgnode.cpp @@ -447,7 +447,7 @@ void RegionNode::verify_can_be_irreducible_entry() const { } #endif //ASSERT -bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) { +void RegionNode::try_clean_mem_phis(PhaseIterGVN* igvn) { // Incremental inlining + PhaseStringOpts sometimes produce: // // cmpP with 1 top input @@ -464,32 +464,59 @@ bool RegionNode::try_clean_mem_phi(PhaseGVN *phase) { // replaced by If's control input but because there's still a Phi, // the Region stays in the graph. The top input from the cmpP is // propagated forward and a subgraph that is useful goes away. The - // code below replaces the Phi with the MergeMem so that the Region - // is simplified. - - PhiNode* phi = has_unique_phi(); - if (phi && phi->type() == Type::MEMORY && req() == 3 && phi->is_diamond_phi(true)) { - MergeMemNode* m = nullptr; - assert(phi->req() == 3, "same as region"); - for (uint i = 1; i < 3; ++i) { - Node *mem = phi->in(i); - if (mem && mem->is_MergeMem() && in(i)->outcnt() == 1) { - // Nothing is control-dependent on path #i except the region itself. - m = mem->as_MergeMem(); - uint j = 3 - i; - Node* other = phi->in(j); - if (other && other == m->base_memory()) { - // m is a successor memory to other, and is not pinned inside the diamond, so push it out. - // This will allow the diamond to collapse completely. - phase->is_IterGVN()->replace_node(phi, m); - return true; - } - } + // code in PhiNode::try_clean_memory_phi() replaces the Phi with the + // MergeMem in order to remove the Region if its last phi dies. + + if (!is_diamond()) { + return; + } + + for (DUIterator_Fast imax, i = fast_outs(imax); i < imax; i++) { + Node* phi = fast_out(i); + if (phi->is_Phi() && phi->as_Phi()->try_clean_memory_phi(igvn)) { + --i; + --imax; } } - return false; } +// Does this region merge a simple diamond formed by a proper IfNode? +// +// Cmp +// / +// ctrl Bool +// \ / +// IfNode +// / \ +// IfFalse IfTrue +// \ / +// Region +bool RegionNode::is_diamond() const { + if (req() != 3) { + return false; + } + + Node* left_path = in(1); + Node* right_path = in(2); + if (left_path == nullptr || right_path == nullptr) { + return false; + } + Node* diamond_if = left_path->in(0); + if (diamond_if == nullptr || !diamond_if->is_If() || diamond_if != right_path->in(0)) { + return false; + } + + // Check for a proper bool/cmp + const Node* bol = diamond_if->in(1); + if (!bol->is_Bool()) { + return false; + } + const Node* cmp = bol->in(1); + if (!cmp->is_Cmp()) { + return false; + } + return true; +} //------------------------------Ideal------------------------------------------ // Return a node which is more "ideal" than the current node. Must preserve // the CFG, but we can still strip out dead paths. @@ -501,10 +528,8 @@ Node *RegionNode::Ideal(PhaseGVN *phase, bool can_reshape) { // arm of the same IF. If found, then the control-flow split is useless. bool has_phis = false; if (can_reshape) { // Need DU info to check for Phi users + try_clean_mem_phis(phase->is_IterGVN()); has_phis = (has_phi() != nullptr); // Cache result - if (has_phis && try_clean_mem_phi(phase)) { - has_phis = false; - } if (!has_phis) { // No Phi users? Nothing merging? for (uint i = 1; i < req()-1; i++) { @@ -1327,42 +1352,59 @@ const Type* PhiNode::Value(PhaseGVN* phase) const { return ft; } - -//------------------------------is_diamond_phi--------------------------------- // Does this Phi represent a simple well-shaped diamond merge? Return the // index of the true path or 0 otherwise. -// If check_control_only is true, do not inspect the If node at the -// top, and return -1 (not an edge number) on success. -int PhiNode::is_diamond_phi(bool check_control_only) const { - // Check for a 2-path merge - Node *region = in(0); - if( !region ) return 0; - if( region->req() != 3 ) return 0; - if( req() != 3 ) return 0; - // Check that both paths come from the same If - Node *ifp1 = region->in(1); - Node *ifp2 = region->in(2); - if( !ifp1 || !ifp2 ) return 0; - Node *iff = ifp1->in(0); - if( !iff || !iff->is_If() ) return 0; - if( iff != ifp2->in(0) ) return 0; - if (check_control_only) return -1; - // Check for a proper bool/cmp - const Node *b = iff->in(1); - if( !b->is_Bool() ) return 0; - const Node *cmp = b->in(1); - if( !cmp->is_Cmp() ) return 0; - - // Check for branching opposite expected - if( ifp2->Opcode() == Op_IfTrue ) { - assert( ifp1->Opcode() == Op_IfFalse, "" ); - return 2; - } else { - assert( ifp1->Opcode() == Op_IfTrue, "" ); +int PhiNode::is_diamond_phi() const { + Node* region = in(0); + assert(region != nullptr && region->is_Region(), "phi must have region"); + if (!region->as_Region()->is_diamond()) { + return 0; + } + + if (region->in(1)->is_IfTrue()) { + assert(region->in(2)->is_IfFalse(), "bad If"); return 1; + } else { + // Flipped projections. + assert(region->in(2)->is_IfTrue(), "bad If"); + return 2; } } +// Do the following transformation if we find this graph shape: +// +// If If +// / \ / \ +// IfFalse IfTrue /- Some Node IfFalse IfTrue +// \ / / / \ / Some Node +// Region / /-MergeMem ===> Region | +// / \---Phi | MergeMem +// [other phis] \ [other phis] | +// use use +bool PhiNode::try_clean_memory_phi(PhaseIterGVN* igvn) { + if (_type != Type::MEMORY) { + return false; + } + assert(is_diamond_phi(), "sanity"); + assert(req() == 3, "same as region"); + const Node* region = in(0); + for (uint i = 1; i < 3; ++i) { + Node* mem = in(i); + if (mem != nullptr && mem->is_MergeMem() && region->in(i)->outcnt() == 1) { + // Nothing is control-dependent on path #i except the region itself. + MergeMemNode* merge_mem = mem->as_MergeMem(); + uint j = 3 - i; + Node* other = in(j); + if (other != nullptr && other == merge_mem->base_memory()) { + // merge_mem is a successor memory to other, and is not pinned inside the diamond, so push it out. + // This will allow the diamond to collapse completely if there are no remaining phis left afterward. + igvn->replace_node(this, merge_mem); + return true; + } + } + } + return false; +} //----------------------------check_cmove_id----------------------------------- // Check for CMove'ing a constant after comparing against the constant. // Happens all the time now, since if we compare equality vs a constant in diff --git a/src/hotspot/share/opto/cfgnode.hpp b/src/hotspot/share/opto/cfgnode.hpp index 5f8d8f0bc15b7..9fc68f56309ad 100644 --- a/src/hotspot/share/opto/cfgnode.hpp +++ b/src/hotspot/share/opto/cfgnode.hpp @@ -132,7 +132,8 @@ class RegionNode : public Node { virtual Node* Ideal(PhaseGVN* phase, bool can_reshape); void remove_unreachable_subgraph(PhaseIterGVN* igvn); virtual const RegMask &out_RegMask() const; - bool try_clean_mem_phi(PhaseGVN* phase); + bool is_diamond() const; + void try_clean_mem_phis(PhaseIterGVN* phase); bool optimize_trichotomy(PhaseIterGVN* igvn); NOT_PRODUCT(virtual void dump_spec(outputStream* st) const;) }; @@ -233,7 +234,8 @@ class PhiNode : public TypeNode { LoopSafety simple_data_loop_check(Node *in) const; // Is it unsafe data loop? It becomes a dead loop if this phi node removed. bool is_unsafe_data_reference(Node *in) const; - int is_diamond_phi(bool check_control_only = false) const; + int is_diamond_phi() const; + bool try_clean_memory_phi(PhaseIterGVN* igvn); virtual int Opcode() const; virtual bool pinned() const { return in(0) != 0; } virtual const TypePtr *adr_type() const { verify_adr_type(true); return _adr_type; } diff --git a/test/hotspot/jtreg/compiler/c2/irTests/igvn/TestCleanMemPhi.java b/test/hotspot/jtreg/compiler/c2/irTests/igvn/TestCleanMemPhi.java new file mode 100644 index 0000000000000..8652f81b369f7 --- /dev/null +++ b/test/hotspot/jtreg/compiler/c2/irTests/igvn/TestCleanMemPhi.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package compiler.c2.irTests.igvn; + +import compiler.lib.ir_framework.*; +import jdk.test.lib.Asserts; + +/* + * @test + * @bug 8314997 + * @requires vm.debug == true & vm.compiler2.enabled + * @summary Test that diamond if-region is removed due to calling try_clean_mem_phi(). + * @library /test/lib / + * @run driver compiler.c2.irTests.igvn.TestCleanMemPhi + */ +public class TestCleanMemPhi { + static boolean flag, flag2; + static int iFld; + + public static void main(String[] args) { + TestFramework testFramework = new TestFramework(); + testFramework.setDefaultWarmup(0); + testFramework.addFlags("-XX:+AlwaysIncrementalInline", "-XX:-PartialPeelLoop", "-XX:-LoopUnswitching"); + testFramework.addScenarios(new Scenario(1, "-XX:-StressIGVN"), + new Scenario(2, "-XX:+StressIGVN")); + testFramework.start(); + } + + static class A { + int i; + } + + + static A a1 = new A(); + static A a2 = new A(); + + + @Test + @IR(counts = {IRNode.COUNTED_LOOP, "> 0"}) + static void testCountedLoop() { + int zero = 34; + + int limit = 2; + for (; limit < 4; limit *= 2) ; + for (int i = 2; i < limit; i++) { + zero = 0; + } + + + // Loop is not converted to a counted loop and cannot be removed (i.e. empty loop removal) because a diamond + // is not removed because we cannot apply try_clean_mem_phi(). + int i = 0; + do { + iFld = 34; + if (flag2) { + iFld++; + } + + int z = 34; + if (flag) { + lateInline(); // Inlined late -> leaves a MergeMem + if (zero == 34) { // False but only cleaned up after CCP + iFld = 38; + } + z = 32; // Ensures to get a diamond If-Region + } + // Region merging a proper diamond after CCP with a memory phi merging loop phi and the MergeMem from lateInline(). + // Region is not added to the IGVN worklist anymore once the second phi dies. + i++; + } while (zero == 34 || (i < 1000 && a1 == a2)); // Could be converted to a counted loop after the diamond is removed after CCP. + } + + @Test + @IR(failOn = IRNode.LOOP) + static void testRemoveLoop() { + int zero = 34; + + int limit = 2; + for (; limit < 4; limit *= 2) ; + for (int i = 2; i < limit; i++) { + zero = 0; + } + + + // Loop is not converted to a counted loop and cannot be removed (i.e. empty loop removal). + int i = 0; + do { + iFld = 34; + + int z = 34; + if (flag) { + lateInline(); + if (zero == 34) { + iFld = 38; + } + z = 32; // Ensures to get a diamond If-Region + } + + i++; + } while (zero == 34 || (i < 21 && a1 == a2)); + } + + static void lateInline() { + } +}