infernap12/jdk
1
0
Fork 0
mirror of https://github.com/openjdk/jdk.git synced 2026-01-28 12:09:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

8347018: C2: Insertion of Assertion Predicates ignores the effects of PhaseIdealLoop::clone_up_backedge_goo()

Browse Source 
Reviewed-by: epeter, kvn
This commit is contained in:
Christian Hagedorn 2025-01-20 12:24:33 +00:00
parent 85fdd2cc12
commit 8a83dc213a
5 changed files with 168 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
src/hotspot/share/opto/loopTransform.cpp
View File

@ -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) {

12
src/hotspot/share/opto/loopnode.hpp
View File

@ -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();

2
src/hotspot/share/opto/predicates.cpp
View File

@ -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
}

51
src/hotspot/share/opto/predicates.hpp
View File

@ -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;
}
};

55
test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java Normal file
View File

@ -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();
}
}
}