From 8a83dc213ac630ec79d62637133fe7aa102a27a3 Mon Sep 17 00:00:00 2001 From: Christian Hagedorn Date: Mon, 20 Jan 2025 12:24:33 +0000 Subject: [PATCH] 8347018: C2: Insertion of Assertion Predicates ignores the effects of PhaseIdealLoop::clone_up_backedge_goo() Reviewed-by: epeter, kvn --- src/hotspot/share/opto/loopTransform.cpp | 74 ++++++++++++++----- src/hotspot/share/opto/loopnode.hpp | 12 ++- src/hotspot/share/opto/predicates.cpp | 2 +- src/hotspot/share/opto/predicates.hpp | 51 +++++++++++-- ...AboveAssertionPredicatesAndUsingStore.java | 55 ++++++++++++++ 5 files changed, 168 insertions(+), 26 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index d8d7be5c89c..d309eec9422 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2000, 2025, 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 @@ -1424,11 +1424,13 @@ void PhaseIdealLoop::insert_pre_post_loops(IdealLoopTree *loop, Node_List &old_n _igvn.hash_delete(outer_main_head); outer_main_head->set_req(LoopNode::EntryControl, min_taken); set_idom(outer_main_head, min_taken, dd_main_head); + assert(post_head->in(1)->is_IfProj(), "must be zero-trip guard If node projection of the post loop"); VectorSet visited; Node_Stack clones(main_head->back_control()->outcnt()); // Step B3: Make the fall-in values to the main-loop come from the // fall-out values of the pre-loop. + const uint last_node_index_in_pre_loop_body = Compile::current()->unique() - 1; for (DUIterator i2 = main_head->outs(); main_head->has_out(i2); i2++) { Node* main_phi = main_head->out(i2); if (main_phi->is_Phi() && main_phi->in(0) == main_head && main_phi->outcnt() > 0) { @@ -1441,21 +1443,13 @@ void PhaseIdealLoop::insert_pre_post_loops(IdealLoopTree *loop, Node_List &old_n main_phi->set_req(LoopNode::EntryControl, fallpre); } } + DEBUG_ONLY(const uint last_node_index_from_backedge_goo = Compile::current()->unique() - 1); - // Nodes inside the loop may be control dependent on a predicate - // that was moved before the preloop. If the back branch of the main - // or post loops becomes dead, those nodes won't be dependent on the - // test that guards that loop nest anymore which could lead to an - // incorrect array access because it executes independently of the - // test that was guarding the loop nest. We add a special CastII on - // the if branch that enters the loop, between the input induction - // variable value and the induction variable Phi to preserve correct - // dependencies. - - assert(post_head->in(1)->is_IfProj(), "must be zero-trip guard If node projection of the post loop"); DEBUG_ONLY(ensure_zero_trip_guard_proj(outer_main_head->in(LoopNode::EntryControl), true);) if (UseLoopPredicate) { - initialize_assertion_predicates_for_main_loop(pre_head, main_head, first_node_index_in_pre_loop_body, old_new); + initialize_assertion_predicates_for_main_loop(pre_head, main_head, first_node_index_in_pre_loop_body, + last_node_index_in_pre_loop_body, + DEBUG_ONLY(last_node_index_from_backedge_goo COMMA) old_new); } // Step B4: Shorten the pre-loop to run only 1 iteration (for now). @@ -1729,10 +1723,15 @@ void PhaseIdealLoop::initialize_assertion_predicates_for_peeled_loop(CountedLoop // Target Loop: Original - main_loop_head void PhaseIdealLoop::initialize_assertion_predicates_for_main_loop(CountedLoopNode* pre_loop_head, CountedLoopNode* main_loop_head, - const uint first_node_index_in_cloned_loop_body, + const uint first_node_index_in_pre_loop_body, + const uint last_node_index_in_pre_loop_body, + DEBUG_ONLY(const uint last_node_index_from_backedge_goo COMMA) const Node_List& old_new) { - const NodeInOriginalLoopBody node_in_original_loop_body(first_node_index_in_cloned_loop_body, old_new); - create_assertion_predicates_at_loop(pre_loop_head, main_loop_head, node_in_original_loop_body, true); + assert(first_node_index_in_pre_loop_body < last_node_index_in_pre_loop_body, "cloned some nodes"); + const NodeInMainLoopBody node_in_main_loop_body(first_node_index_in_pre_loop_body, + last_node_index_in_pre_loop_body, + DEBUG_ONLY(last_node_index_from_backedge_goo COMMA) old_new); + create_assertion_predicates_at_main_or_post_loop(pre_loop_head, main_loop_head, node_in_main_loop_body, true); } // Source Loop: Original - main_loop_head @@ -1741,7 +1740,7 @@ void PhaseIdealLoop::initialize_assertion_predicates_for_post_loop(CountedLoopNo CountedLoopNode* post_loop_head, const uint first_node_index_in_cloned_loop_body) { const NodeInClonedLoopBody node_in_cloned_loop_body(first_node_index_in_cloned_loop_body); - create_assertion_predicates_at_loop(main_loop_head, post_loop_head, node_in_cloned_loop_body, false); + create_assertion_predicates_at_main_or_post_loop(main_loop_head, post_loop_head, node_in_cloned_loop_body, false); } void PhaseIdealLoop::create_assertion_predicates_at_loop(CountedLoopNode* source_loop_head, @@ -1754,6 +1753,47 @@ void PhaseIdealLoop::create_assertion_predicates_at_loop(CountedLoopNode* source PredicateIterator predicate_iterator(source_loop_entry); predicate_iterator.for_each(create_assertion_predicates_visitor); } + +void PhaseIdealLoop::create_assertion_predicates_at_main_or_post_loop(CountedLoopNode* source_loop_head, + CountedLoopNode* target_loop_head, + const NodeInLoopBody& _node_in_loop_body, + bool clone_template) { + Node* old_target_loop_head_entry = target_loop_head->skip_strip_mined()->in(LoopNode::EntryControl); + const uint node_index_before_new_assertion_predicate_nodes = C->unique(); + const bool need_to_rewire_old_target_loop_entry_dependencies = old_target_loop_head_entry->outcnt() > 1; + create_assertion_predicates_at_loop(source_loop_head, target_loop_head, _node_in_loop_body, clone_template); + if (need_to_rewire_old_target_loop_entry_dependencies) { + rewire_old_target_loop_entry_dependency_to_new_entry(target_loop_head, old_target_loop_head_entry, + node_index_before_new_assertion_predicate_nodes); + } +} + +// Rewire any control dependent nodes on the old target loop entry before adding Assertion Predicate related nodes. +// These have been added by PhaseIdealLoop::clone_up_backedge_goo() and assume to be ending up at the target loop entry +// which is no longer the case when adding additional Assertion Predicates. Fix this by rewiring these nodes to the new +// target loop entry which corresponds to the tail of the last Assertion Predicate before the target loop. This is safe +// to do because these control dependent nodes on the old target loop entry created by clone_up_backedge_goo() were +// pinned on the loop backedge before. The Assertion Predicates are not control dependent on these nodes in any way. +void PhaseIdealLoop::rewire_old_target_loop_entry_dependency_to_new_entry( + LoopNode* target_loop_head, const Node* old_target_loop_entry, + const uint node_index_before_new_assertion_predicate_nodes) { + Node* new_main_loop_entry = target_loop_head->skip_strip_mined()->in(LoopNode::EntryControl); + if (new_main_loop_entry == old_target_loop_entry) { + // No Assertion Predicates added. + return; + } + + for (DUIterator_Fast imax, i = old_target_loop_entry->fast_outs(imax); i < imax; i++) { + Node* out = old_target_loop_entry->fast_out(i); + if (!out->is_CFG() && out->_idx < node_index_before_new_assertion_predicate_nodes) { + _igvn.replace_input_of(out, 0, new_main_loop_entry); + set_ctrl(out, new_main_loop_entry); + --i; + --imax; + } + } +} + //------------------------------do_unroll-------------------------------------- // Unroll the loop body one step - make each trip do 2 iterations. void PhaseIdealLoop::do_unroll(IdealLoopTree *loop, Node_List &old_new, bool adjust_min_trip) { diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index 829c3cb8a18..91508c512cb 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2025, 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 @@ -951,12 +951,20 @@ private: const Node_List& old_new); void initialize_assertion_predicates_for_main_loop(CountedLoopNode* pre_loop_head, CountedLoopNode* main_loop_head, - uint first_node_index_in_cloned_loop_body, + uint first_node_index_in_pre_loop_body, + uint last_node_index_in_pre_loop_body, + DEBUG_ONLY(uint last_node_index_from_backedge_goo COMMA) const Node_List& old_new); void initialize_assertion_predicates_for_post_loop(CountedLoopNode* main_loop_head, CountedLoopNode* post_loop_head, uint first_node_index_in_cloned_loop_body); void create_assertion_predicates_at_loop(CountedLoopNode* source_loop_head, CountedLoopNode* target_loop_head, const NodeInLoopBody& _node_in_loop_body, bool clone_template); + void create_assertion_predicates_at_main_or_post_loop(CountedLoopNode* source_loop_head, + CountedLoopNode* target_loop_head, + const NodeInLoopBody& _node_in_loop_body, bool clone_template); + void rewire_old_target_loop_entry_dependency_to_new_entry(LoopNode* target_loop_head, + const Node* old_target_loop_entry, + uint node_index_before_new_assertion_predicate_nodes); void insert_loop_limit_check_predicate(ParsePredicateSuccessProj* loop_limit_check_parse_proj, Node* cmp_limit, Node* bol); void log_loop_tree(); diff --git a/src/hotspot/share/opto/predicates.cpp b/src/hotspot/share/opto/predicates.cpp index fe46cd6e427..6343a717757 100644 --- a/src/hotspot/share/opto/predicates.cpp +++ b/src/hotspot/share/opto/predicates.cpp @@ -154,7 +154,7 @@ void TemplateAssertionPredicate::rewire_loop_data_dependencies(IfTrueNode* targe PhaseIdealLoop* phase) const { for (DUIterator i = _success_proj->outs(); _success_proj->has_out(i); i++) { Node* output = _success_proj->out(i); - if (!output->is_CFG() && data_in_loop_body.check(output)) { + if (!output->is_CFG() && data_in_loop_body.check_node_in_loop_body(output)) { phase->igvn().replace_input_of(output, 0, target_predicate); --i; // account for the just deleted output } diff --git a/src/hotspot/share/opto/predicates.hpp b/src/hotspot/share/opto/predicates.hpp index 3044b08a9f8..22de46ea561 100644 --- a/src/hotspot/share/opto/predicates.hpp +++ b/src/hotspot/share/opto/predicates.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2023, 2025, 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 @@ -248,7 +248,7 @@ class PredicateVisitor : public StackObj { // Interface to check whether a node is in a loop body or not. class NodeInLoopBody : public StackObj { public: - virtual bool check(Node* node) const = 0; + virtual bool check_node_in_loop_body(Node* node) const = 0; }; // Class to represent Assertion Predicates (i.e. either Initialized and/or Template Assertion Predicates). @@ -963,13 +963,52 @@ class NodeInOriginalLoopBody : public NodeInLoopBody { // Check if 'node' is not a cloned node (i.e. "< _first_node_index_in_cloned_loop_body") and if we've created a // clone from 'node' (i.e. _old_new entry is non-null). Then we know that 'node' belongs to the original loop body. - bool check(Node* node) const override { + bool check_node_in_loop_body(Node* node) const override { if (node->_idx < _first_node_index_in_cloned_loop_body) { Node* cloned_node = _old_new[node->_idx]; + // Check that the clone is actually part of the cloned loop body and not from some earlier cloning. return cloned_node != nullptr && cloned_node->_idx >= _first_node_index_in_cloned_loop_body; - } else { - return false; } + return false; + } +}; + +// This class checks whether a node is in the main loop body and not the pre loop body. We cannot use the +// NodeInOriginalLoopBody class because PhaseIdealLoop::clone_up_backedge_goo() could clone additional nodes that +// should be pinned at the main loop body entry. The check in NodeInOriginalLoopBody will ignore these. +class NodeInMainLoopBody : public NodeInLoopBody { + const uint _first_node_index_in_pre_loop_body; + const uint _last_node_index_in_pre_loop_body; + DEBUG_ONLY(const uint _last_node_index_from_backedge_goo;) + const Node_List& _old_new; + + public: + NodeInMainLoopBody(const uint first_node_index_in_pre_loop_body, const uint last_node_index_in_pre_loop_body, + DEBUG_ONLY(const uint last_node_index_from_backedge_goo COMMA) const Node_List& old_new) + : _first_node_index_in_pre_loop_body(first_node_index_in_pre_loop_body), + _last_node_index_in_pre_loop_body(last_node_index_in_pre_loop_body), + DEBUG_ONLY(_last_node_index_from_backedge_goo(last_node_index_from_backedge_goo) COMMA) + _old_new(old_new) {} + NONCOPYABLE(NodeInMainLoopBody); + + // Check if 'node' is not a cloned node (i.e. "< _first_node_index_in_cloned_loop_body") and if we've created a + // clone from 'node' (i.e. _old_new entry is non-null). Then we know that 'node' belongs to the original loop body. + // Additionally check if a node was cloned after the pre loop was created. This indicates that it was created by + // PhaseIdealLoop::clone_up_backedge_goo(). These nodes should also be pinned at the main loop entry. + bool check_node_in_loop_body(Node* node) const override { + if (node->_idx < _first_node_index_in_pre_loop_body) { + Node* cloned_node = _old_new[node->_idx]; + // Check that the clone is actually part of the cloned loop body and not from some earlier cloning. + bool cloned_node_in_pre_loop_body = cloned_node != nullptr && cloned_node->_idx >= _first_node_index_in_pre_loop_body; + assert(!cloned_node_in_pre_loop_body || cloned_node->_idx <= _last_node_index_in_pre_loop_body, + "clone must be part of pre loop body"); + return cloned_node_in_pre_loop_body; + } + // Created in PhaseIdealLoop::clone_up_backedge_goo()? + bool node_created_by_backedge_goo = node->_idx > _last_node_index_in_pre_loop_body; + assert(!node_created_by_backedge_goo || node->_idx <= _last_node_index_from_backedge_goo, + "cloned node must have been created in PhaseIdealLoop::clone_up_backedge_goo()"); + return node_created_by_backedge_goo; } }; @@ -984,7 +1023,7 @@ class NodeInClonedLoopBody : public NodeInLoopBody { // Check if 'node' is a clone. This can easily be achieved by comparing its node index to the first node index // inside the cloned loop body (all of them are clones). - bool check(Node* node) const override { + bool check_node_in_loop_body(Node* node) const override { return node->_idx >= _first_node_index_in_cloned_loop_body; } }; diff --git a/test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java b/test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java new file mode 100644 index 00000000000..0eb5c27cd82 --- /dev/null +++ b/test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java @@ -0,0 +1,55 @@ +/* + * Copyright (c) 2025, 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. + * + */ + +/* + * @test + * @bug 8347018 + * @summary Test that stores cloned with clone_up_backedge_goo() are not pinned above Assertion Predicates on which a + * load node is pinned at which will later fail in scheduling. + * @run main/othervm -Xbatch -XX:CompileCommand=compileonly,*TestLoadPinnedAboveAssertionPredicatesAndUsingStore::test + * compiler.predicates.assertion.TestLoadPinnedAboveAssertionPredicatesAndUsingStore + */ + +package compiler.predicates.assertion; + +public class TestLoadPinnedAboveAssertionPredicatesAndUsingStore { + static int iFld; + static int iArr[] = new int[100]; + + static void test() { + int i = 63; + do { + iArr[1] = 34; + iArr[i] += iFld; + for (int j = i; j < 1; j++) { + } + } while (--i > 0); + } + + public static void main(String[] strArr) { + for (int i = 0; i < 10000; i++) { + test(); + } + } +}