From e203df46faf610e35e2c2510271ad68199f4fa3f Mon Sep 17 00:00:00 2001 From: Roland Westrelin Date: Thu, 5 Sep 2024 15:51:27 +0000 Subject: [PATCH] 8338100: C2: assert(!n_loop->is_member(get_loop(lca))) failed: control must not be back in the loop Co-authored-by: Emanuel Peter Reviewed-by: chagedorn, thartmann --- src/hotspot/share/opto/loopnode.cpp | 80 ++++++++----------- src/hotspot/share/opto/loopnode.hpp | 6 +- src/hotspot/share/opto/parse1.cpp | 2 +- .../LongCountedLoopInInfiniteLoop.jasm | 76 ++++++++++++++++++ .../loopopts/MoveStoreAfterInfiniteLoop.jasm | 74 +++++++++++++++++ .../TestLongCountedLoopInInfiniteLoop.java | 45 +++++++++++ .../TestMoveStoreAfterInfiniteLoop.java | 44 ++++++++++ 7 files changed, 276 insertions(+), 51 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/loopopts/LongCountedLoopInInfiniteLoop.jasm create mode 100644 test/hotspot/jtreg/compiler/loopopts/MoveStoreAfterInfiniteLoop.jasm create mode 100644 test/hotspot/jtreg/compiler/loopopts/TestLongCountedLoopInInfiniteLoop.java create mode 100644 test/hotspot/jtreg/compiler/loopopts/TestMoveStoreAfterInfiniteLoop.java diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index b4c134570e6..3128e23d79c 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -4511,23 +4511,6 @@ bool PhaseIdealLoop::process_expensive_nodes() { return progress; } -#ifdef ASSERT -// Goes over all children of the root of the loop tree. Check if any of them have a path -// down to Root, that does not go via a NeverBranch exit. -bool PhaseIdealLoop::only_has_infinite_loops() { - ResourceMark rm; - Unique_Node_List worklist; - // start traversal at all loop heads of first-level loops - for (IdealLoopTree* l = _ltree_root->_child; l != nullptr; l = l->_next) { - Node* head = l->_head; - assert(head->is_Region(), ""); - worklist.push(head); - } - return RegionNode::are_all_nodes_in_infinite_subgraph(worklist); -} -#endif - - //============================================================================= //----------------------------build_and_optimize------------------------------- // Create a PhaseLoop. Build the ideal Loop tree. Map each Ideal Node to @@ -4586,13 +4569,9 @@ void PhaseIdealLoop::build_and_optimize() { return; } - // Verify that the has_loops() flag set at parse time is consistent - // with the just built loop tree. With infinite loops, it could be - // that one pass of loop opts only finds infinite loops, clears the - // has_loops() flag but adds NeverBranch nodes so the next loop opts - // verification pass finds a non empty loop tree. When the back edge + // Verify that the has_loops() flag set at parse time is consistent with the just built loop tree. When the back edge // is an exception edge, parsing doesn't set has_loops(). - assert(_ltree_root->_child == nullptr || C->has_loops() || only_has_infinite_loops() || C->has_exception_backedge(), "parsing found no loops but there are some"); + assert(_ltree_root->_child == nullptr || C->has_loops() || C->has_exception_backedge(), "parsing found no loops but there are some"); // No loops after all if( !_ltree_root->_child && !_verify_only ) C->set_has_loops(false); @@ -5425,7 +5404,7 @@ void PhaseIdealLoop::build_loop_tree() { if ( bltstack.length() == stack_size ) { // There were no additional children, post visit node now (void)bltstack.pop(); // Remove node from stack - pre_order = build_loop_tree_impl( n, pre_order ); + pre_order = build_loop_tree_impl(n, pre_order); // Check for bailout if (C->failing()) { return; @@ -5443,7 +5422,7 @@ void PhaseIdealLoop::build_loop_tree() { } //------------------------------build_loop_tree_impl--------------------------- -int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { +int PhaseIdealLoop::build_loop_tree_impl(Node* n, int pre_order) { // ---- Post-pass Work ---- // Pre-walked but not post-walked nodes need a pre_order number. @@ -5454,24 +5433,24 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { // for it. Then find the tightest enclosing loop for the self Node. for (DUIterator_Fast imax, i = n->fast_outs(imax); i < imax; i++) { Node* m = n->fast_out(i); // Child - if( n == m ) continue; // Ignore control self-cycles - if( !m->is_CFG() ) continue;// Ignore non-CFG edges + if (n == m) continue; // Ignore control self-cycles + if (!m->is_CFG()) continue;// Ignore non-CFG edges IdealLoopTree *l; // Child's loop - if( !is_postvisited(m) ) { // Child visited but not post-visited? + if (!is_postvisited(m)) { // Child visited but not post-visited? // Found a backedge - assert( get_preorder(m) < pre_order, "should be backedge" ); + assert(get_preorder(m) < pre_order, "should be backedge"); // Check for the RootNode, which is already a LoopNode and is allowed // to have multiple "backedges". - if( m == C->root()) { // Found the root? + if (m == C->root()) { // Found the root? l = _ltree_root; // Root is the outermost LoopNode } else { // Else found a nested loop // Insert a LoopNode to mark this loop. l = new IdealLoopTree(this, m, n); } // End of Else found a nested loop - if( !has_loop(m) ) // If 'm' does not already have a loop set + if (!has_loop(m)) { // If 'm' does not already have a loop set set_loop(m, l); // Set loop header to loop now - + } } else { // Else not a nested loop if (!_loop_or_ctrl[m->_idx]) continue; // Dead code has no loop IdealLoopTree* m_loop = get_loop(m); @@ -5480,23 +5459,17 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { // is a member of some outer enclosing loop. Since there are no // shared headers (I've split them already) I only need to go up // at most 1 level. - while( l && l->_head == m ) // Successor heads loop? + while (l && l->_head == m) { // Successor heads loop? l = l->_parent; // Move up 1 for me + } // If this loop is not properly parented, then this loop // has no exit path out, i.e. its an infinite loop. - if( !l ) { + if (!l) { // Make loop "reachable" from root so the CFG is reachable. Basically // insert a bogus loop exit that is never taken. 'm', the loop head, // points to 'n', one (of possibly many) fall-in paths. There may be // many backedges as well. - // Here I set the loop to be the root loop. I could have, after - // inserting a bogus loop exit, restarted the recursion and found my - // new loop exit. This would make the infinite loop a first-class - // loop and it would then get properly optimized. What's the use of - // optimizing an infinite loop? - l = _ltree_root; // Oops, found infinite loop - if (!_verify_only) { // Insert the NeverBranch between 'm' and it's control user. NeverBranchNode *iff = new NeverBranchNode( m ); @@ -5520,7 +5493,7 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { // Now create the never-taken loop exit Node *if_f = new CProjNode( iff, 1 ); _igvn.register_new_node_with_optimizer(if_f); - set_loop(if_f, l); + set_loop(if_f, _ltree_root); // Find frame ptr for Halt. Relies on the optimizer // V-N'ing. Easier and quicker than searching through // the program structure. @@ -5529,10 +5502,27 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { // Halt & Catch Fire Node* halt = new HaltNode(if_f, frame, "never-taken loop exit reached"); _igvn.register_new_node_with_optimizer(halt); - set_loop(halt, l); + set_loop(halt, _ltree_root); _igvn.add_input_to(C->root(), halt); } set_loop(C->root(), _ltree_root); + // move to outer most loop with same header + l = m_loop; + while (true) { + IdealLoopTree* next = l->_parent; + if (next == nullptr || next->_head != m) { + break; + } + l = next; + } + // properly insert infinite loop in loop tree + sort(_ltree_root, l); + // fix child link from parent + IdealLoopTree* p = l->_parent; + l->_next = p->_child; + p->_child = l; + // code below needs enclosing loop + l = l->_parent; } } if (is_postvisited(l->_head)) { @@ -5586,7 +5576,7 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { assert( get_loop(n) == innermost, "" ); IdealLoopTree *p = innermost->_parent; IdealLoopTree *l = innermost; - while( p && l->_head == n ) { + while (p && l->_head == n) { l->_next = p->_child; // Put self on parents 'next child' p->_child = l; // Make self as first child of parent l = p; // Now walk up the parent chain @@ -5600,7 +5590,7 @@ int PhaseIdealLoop::build_loop_tree_impl( Node *n, int pre_order ) { // Record tightest enclosing loop for self. Mark as post-visited. set_loop(n, innermost); // Also record has_call flag early on - if( innermost ) { + if (innermost) { if( n->is_Call() && !n->is_CallLeaf() && !n->is_macro() ) { // Do not count uncommon calls if( !n->is_CallStaticJava() || !n->as_CallStaticJava()->_name ) { diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index 369f446adbb..6b2ad120a63 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -966,10 +966,6 @@ private: const Node_List& old_new); void insert_loop_limit_check_predicate(ParsePredicateSuccessProj* loop_limit_check_parse_proj, Node* cmp_limit, Node* bol); -#ifdef ASSERT - bool only_has_infinite_loops(); -#endif - void log_loop_tree(); public: @@ -1076,7 +1072,7 @@ private: // Place 'n' in some loop nest, where 'n' is a CFG node void build_loop_tree(); - int build_loop_tree_impl( Node *n, int pre_order ); + int build_loop_tree_impl(Node* n, int pre_order); // Insert loop into the existing loop tree. 'innermost' is a leaf of the // loop tree, not the root. IdealLoopTree *sort( IdealLoopTree *loop, IdealLoopTree *innermost ); diff --git a/src/hotspot/share/opto/parse1.cpp b/src/hotspot/share/opto/parse1.cpp index 50ccc147322..d4a6a9ce5b7 100644 --- a/src/hotspot/share/opto/parse1.cpp +++ b/src/hotspot/share/opto/parse1.cpp @@ -1651,7 +1651,7 @@ void Parse::merge_new_path(int target_bci) { // The ex_oop must be pushed on the stack, unlike throw_to_exit. void Parse::merge_exception(int target_bci) { #ifdef ASSERT - if (target_bci < bci()) { + if (target_bci <= bci()) { C->set_exception_backedge(); } #endif diff --git a/test/hotspot/jtreg/compiler/loopopts/LongCountedLoopInInfiniteLoop.jasm b/test/hotspot/jtreg/compiler/loopopts/LongCountedLoopInInfiniteLoop.jasm new file mode 100644 index 00000000000..e7f75d46b5b --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/LongCountedLoopInInfiniteLoop.jasm @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2024, 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. + */ + +super public class LongCountedLoopInInfiniteLoop +{ + public Method "":"()V" + stack 1 locals 1 + { + aload_0; + invokespecial Method java/lang/Object."":"()V"; + return; + } + Method test:"()V" + stack 3 locals 3 + { + // #1 = 0; + iconst_0; + istore_1; + + LOOPa: + // if #1 >= 10: goto END + iload_1; + bipush 10; + if_icmpge END; + + // if #1 > 1: goto LOOPc + iload_1; + iconst_1; + if_icmpgt LOOPc; + + // #2 = 0; + iconst_0; + istore_2; + + LOOPb: + // if #2 > 2: goto LOOPa + iload_2; + iconst_2; + if_icmpgt LOOPa; + + // #2 ++ + iinc 2, 1; + + goto LOOPb; + + LOOPc: + // #1 ++ + iinc 1, 1; + + goto LOOPa; + + END: + return; + + } +} diff --git a/test/hotspot/jtreg/compiler/loopopts/MoveStoreAfterInfiniteLoop.jasm b/test/hotspot/jtreg/compiler/loopopts/MoveStoreAfterInfiniteLoop.jasm new file mode 100644 index 00000000000..f93a2aa1170 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/MoveStoreAfterInfiniteLoop.jasm @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2024, 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. + */ + +super public class MoveStoreAfterInfiniteLoop +{ + static Field a:I; + static Field b:I; + static Field c:S; + + public Method "":"()V" + stack 1 locals 1 + { + aload_0; + invokespecial Method java/lang/Object."":"()V"; + return; + } + +public static Method test:"()V" + stack 3 locals 3 + { + LTOP: + iconst_0; + istore_1; + + LOUTER: + iload_1; + bipush 10; + if_icmpge LTOP; + + getstatic Field c:"S"; + putstatic Field a:"I"; + + iconst_0; + istore_2; + + LINNER: + iload_2; + iconst_2; + if_icmpge LBOTTOM; + + getstatic Field b:"I"; + i2s; + putstatic Field c:"S"; + + iinc 2, 1; + + goto LINNER; + + LBOTTOM: + iinc 1, 1; + + goto LOUTER; + } +} diff --git a/test/hotspot/jtreg/compiler/loopopts/TestLongCountedLoopInInfiniteLoop.java b/test/hotspot/jtreg/compiler/loopopts/TestLongCountedLoopInInfiniteLoop.java new file mode 100644 index 00000000000..4dc16797258 --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/TestLongCountedLoopInInfiniteLoop.java @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. 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. + */ + +/* + * @test + * @bug 8336478 + * @summary C2: assert(!n->as_Loop()->is_loop_nest_inner_loop() || _loop_opts_cnt == 0) failed: should have been turned into a counted loop + * @compile LongCountedLoopInInfiniteLoop.jasm + * @run main/othervm -XX:+UnlockExperimentalVMOptions -Xcomp -XX:PerMethodTrapLimit=0 -XX:PerMethodSpecTrapLimit=0 + * -XX:+IgnoreUnrecognizedVMOptions -XX:StressLongCountedLoop=2000000 + * -XX:CompileCommand=compileonly,TestLongCountedLoopInInfiniteLoop::test TestLongCountedLoopInInfiniteLoop + */ + +public class TestLongCountedLoopInInfiniteLoop { + public static void main(String[] args) { + LongCountedLoopInInfiniteLoop obj = new LongCountedLoopInInfiniteLoop(); + test(false, obj); + } + + private static void test(boolean flag, LongCountedLoopInInfiniteLoop obj) { + if (flag) { + obj.test(); + } + } +} diff --git a/test/hotspot/jtreg/compiler/loopopts/TestMoveStoreAfterInfiniteLoop.java b/test/hotspot/jtreg/compiler/loopopts/TestMoveStoreAfterInfiniteLoop.java new file mode 100644 index 00000000000..322e7a5053c --- /dev/null +++ b/test/hotspot/jtreg/compiler/loopopts/TestMoveStoreAfterInfiniteLoop.java @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2024, Red Hat, Inc. 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. + */ + +/* + * @test + * @bug 8338100 + * @summary C2: assert(!n_loop->is_member(get_loop(lca))) failed: control must not be back in the loop + * @compile MoveStoreAfterInfiniteLoop.jasm + * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:CompileOnly=TestMoveStoreAfterInfiniteLoop::test + * -XX:CompileCommand=inline,MoveStoreAfterInfiniteLoop::test TestMoveStoreAfterInfiniteLoop + */ + +public class TestMoveStoreAfterInfiniteLoop { + public static void main(String[] args) { + new MoveStoreAfterInfiniteLoop(); + test(false); + } + + private static void test(boolean flag) { + if (flag) { + MoveStoreAfterInfiniteLoop.test(); + } + } +}