From 35f4a7410cdaaa9d3ce68148cb81e893ad0d93de Mon Sep 17 00:00:00 2001 From: Roland Westrelin Date: Tue, 25 Nov 2025 13:00:07 +0000 Subject: [PATCH] 8366888: C2: incorrect assertion predicate with short running long counted loop Co-authored-by: Christian Hagedorn Reviewed-by: chagedorn, bmaillard --- src/hotspot/share/opto/loopTransform.cpp | 1 - src/hotspot/share/opto/loopnode.cpp | 38 ++++++++- src/hotspot/share/opto/loopnode.hpp | 2 + src/hotspot/share/opto/node.hpp | 7 +- src/hotspot/share/opto/predicates.cpp | 75 +++++++++++++++-- src/hotspot/share/opto/predicates.hpp | 20 +++++ ...untedLoopWithLongRCBadAssertPredicate.java | 81 +++++++++++++++++++ ...ntedLoopWithLongRCBadAssertPredicate2.java | 54 +++++++++++++ 8 files changed, 266 insertions(+), 12 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate.java create mode 100644 test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate2.java diff --git a/src/hotspot/share/opto/loopTransform.cpp b/src/hotspot/share/opto/loopTransform.cpp index 31d1cbe0443..5c65103677b 100644 --- a/src/hotspot/share/opto/loopTransform.cpp +++ b/src/hotspot/share/opto/loopTransform.cpp @@ -1411,7 +1411,6 @@ void PhaseIdealLoop::insert_pre_post_loops(IdealLoopTree *loop, Node_List &old_n C->print_method(PHASE_BEFORE_PRE_MAIN_POST, 4, main_head); - Node *pre_header= main_head->in(LoopNode::EntryControl); Node *init = main_head->init_trip(); Node *incr = main_end ->incr(); Node *limit = main_end ->limit(); diff --git a/src/hotspot/share/opto/loopnode.cpp b/src/hotspot/share/opto/loopnode.cpp index dfff7ef96a5..03cc5cbcff6 100644 --- a/src/hotspot/share/opto/loopnode.cpp +++ b/src/hotspot/share/opto/loopnode.cpp @@ -1162,13 +1162,16 @@ bool PhaseIdealLoop::create_loop_nest(IdealLoopTree* loop, Node_List &old_new) { class CloneShortLoopPredicateVisitor : public PredicateVisitor { ClonePredicateToTargetLoop _clone_predicate_to_loop; PhaseIdealLoop* const _phase; + Node* const _new_init; public: CloneShortLoopPredicateVisitor(LoopNode* target_loop_head, + Node* new_init, const NodeInSingleLoopBody &node_in_loop_body, PhaseIdealLoop* phase) : _clone_predicate_to_loop(target_loop_head, node_in_loop_body, phase), - _phase(phase) { + _phase(phase), + _new_init(new_init) { } NONCOPYABLE(CloneShortLoopPredicateVisitor); @@ -1180,11 +1183,32 @@ public: } void visit(const TemplateAssertionPredicate& template_assertion_predicate) override { - _clone_predicate_to_loop.clone_template_assertion_predicate(template_assertion_predicate); + _clone_predicate_to_loop.clone_template_assertion_predicate_and_replace_init(template_assertion_predicate, _new_init); template_assertion_predicate.kill(_phase->igvn()); } }; +// For an int counted loop, try_make_short_running_loop() transforms the loop from: +// for (int = start; i < stop; i+= stride) { ... } +// to +// for (int = 0; i < stop - start; i+= stride) { ... } +// Template Assertion Predicates added so far were with an init value of start. They need to be updated with the new +// init value of 0 (otherwise when a template assertion predicate is turned into an initialized assertion predicate, it +// performs an incorrect check): +// zero +// init | +// | ===> OpaqueLoopInit init +// OpaqueLoopInit \ / +// AddI +// +Node* PhaseIdealLoop::new_assertion_predicate_opaque_init(Node* entry_control, Node* init, Node* int_zero) { + OpaqueLoopInitNode* new_opaque_init = new OpaqueLoopInitNode(C, int_zero); + register_new_node(new_opaque_init, entry_control); + Node* new_init = new AddINode(new_opaque_init, init); + register_new_node(new_init, entry_control); + return new_init; +} + // If the loop is either statically known to run for a small enough number of iterations or if profile data indicates // that, we don't want an outer loop because the overhead of having an outer loop whose backedge is never taken, has a // measurable cost. Furthermore, creating the loop nest usually causes one iteration of the loop to be peeled so @@ -1236,6 +1260,7 @@ bool PhaseIdealLoop::try_make_short_running_loop(IdealLoopTree* loop, jint strid } register_new_node(new_limit, entry_control); + Node* int_zero = intcon(0); PhiNode* phi = head->phi()->as_Phi(); if (profile_short_running_loop) { // Add a Short Running Long Loop Predicate. It's the first predicate in the predicate chain before entering a loop @@ -1261,9 +1286,11 @@ bool PhaseIdealLoop::try_make_short_running_loop(IdealLoopTree* loop, jint strid if (!short_running_long_loop_predicate_block->has_parse_predicate()) { // already trapped return false; } + Node* new_init = new_assertion_predicate_opaque_init(entry_control, init, int_zero); + PredicateIterator predicate_iterator(entry_control); NodeInSingleLoopBody node_in_short_loop_body(this, loop); - CloneShortLoopPredicateVisitor clone_short_loop_predicates_visitor(head, node_in_short_loop_body, this); + CloneShortLoopPredicateVisitor clone_short_loop_predicates_visitor(head, new_init, node_in_short_loop_body, this); predicate_iterator.for_each(clone_short_loop_predicates_visitor); entry_control = head->skip_strip_mined()->in(LoopNode::EntryControl); @@ -1311,6 +1338,10 @@ bool PhaseIdealLoop::try_make_short_running_loop(IdealLoopTree* loop, jint strid register_new_node(new_limit, predicates.entry()); } else { assert(bt == T_INT && known_short_running_loop, "only CountedLoop statically known to be short running"); + PredicateIterator predicate_iterator(entry_control); + Node* new_init = new_assertion_predicate_opaque_init(entry_control, init, int_zero); + UpdateInitForTemplateAssertionPredicates update_init_for_template_assertion_predicates(new_init, this); + predicate_iterator.for_each(update_init_for_template_assertion_predicates); } IfNode* exit_test = head->loopexit(); @@ -1320,7 +1351,6 @@ bool PhaseIdealLoop::try_make_short_running_loop(IdealLoopTree* loop, jint strid register_new_node(new_limit, entry_control); } - Node* int_zero = intcon(0); if (stride_con < 0) { new_limit = new SubINode(int_zero, new_limit); register_new_node(new_limit, entry_control); diff --git a/src/hotspot/share/opto/loopnode.hpp b/src/hotspot/share/opto/loopnode.hpp index 1e34331f213..3b97d76773f 100644 --- a/src/hotspot/share/opto/loopnode.hpp +++ b/src/hotspot/share/opto/loopnode.hpp @@ -1969,6 +1969,8 @@ public: Node* ensure_node_and_inputs_are_above_pre_end(CountedLoopEndNode* pre_end, Node* node); + Node* new_assertion_predicate_opaque_init(Node* entry_control, Node* init, Node* int_zero); + bool try_make_short_running_loop(IdealLoopTree* loop, jint stride_con, const Node_List& range_checks, const uint iters_limit); ConINode* intcon(jint i); diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp index 6067bcbac8d..ec80fb6a0ab 100644 --- a/src/hotspot/share/opto/node.hpp +++ b/src/hotspot/share/opto/node.hpp @@ -2176,7 +2176,10 @@ class BFSActions : public StackObj { virtual bool is_target_node(Node* node) const = 0; // Defines an action that should be taken when we visit a target node in the BFS traversal. - virtual void target_node_action(Node* target_node) = 0; + // To give more freedom, we pass the direct child node to the target node such that + // child->in(i) == target node. This allows to also directly replace the target node instead + // of only updating its inputs. + virtual void target_node_action(Node* child, uint i) = 0; }; // Class to perform a BFS traversal on the data nodes from a given start node. The provided BFSActions guide which @@ -2198,7 +2201,7 @@ class DataNodeBFS : public StackObj { Node* input = next->in(j); if (_bfs_actions.is_target_node(input)) { assert(_bfs_actions.should_visit(input), "must also pass node filter"); - _bfs_actions.target_node_action(input); + _bfs_actions.target_node_action(next, j); } else if (_bfs_actions.should_visit(input)) { _nodes_to_visit.push(input); } diff --git a/src/hotspot/share/opto/predicates.cpp b/src/hotspot/share/opto/predicates.cpp index 208bd6583c5..2489ff563a9 100644 --- a/src/hotspot/share/opto/predicates.cpp +++ b/src/hotspot/share/opto/predicates.cpp @@ -198,12 +198,21 @@ TemplateAssertionPredicate TemplateAssertionPredicate::clone_and_replace_opaque_ Node* new_opaque_input, CountedLoopNode* new_loop_node, PhaseIdealLoop* phase) const { - DEBUG_ONLY(verify();) OpaqueLoopInitNode* new_opaque_init = new OpaqueLoopInitNode(phase->C, new_opaque_input); phase->register_new_node(new_opaque_init, new_control); + return clone_and_replace_init(new_control, new_opaque_init, new_loop_node, phase); +} + +// Clone this Template Assertion Predicate and replace the old OpaqueLoopInit node with 'new_init'. +// Note: 'new_init' could also have the 'OpaqueLoopInit` as parent node further up. +TemplateAssertionPredicate TemplateAssertionPredicate::clone_and_replace_init(Node* new_control, + Node* new_init, + CountedLoopNode* new_loop_node, + PhaseIdealLoop* phase) const { + DEBUG_ONLY(verify();) TemplateAssertionExpression template_assertion_expression(opaque_node(), phase); OpaqueTemplateAssertionPredicateNode* new_opaque_node = - template_assertion_expression.clone_and_replace_init(new_control, new_opaque_init, new_loop_node); + template_assertion_expression.clone_and_replace_init(new_control, new_init, new_loop_node); AssertionPredicateIfCreator assertion_predicate_if_creator(phase); IfTrueNode* success_proj = assertion_predicate_if_creator.create_for_template(new_control, _if_node->Opcode(), new_opaque_node, @@ -238,8 +247,40 @@ class ReplaceOpaqueStrideInput : public BFSActions { return node->is_OpaqueLoopStride(); } - void target_node_action(Node* target_node) override { - _igvn.replace_input_of(target_node, 1, _new_opaque_stride_input); + void target_node_action(Node* child, uint i) override { + assert(child->in(i)->is_OpaqueLoopStride(), "must be OpaqueLoopStride"); + _igvn.replace_input_of(child->in(i), 1, _new_opaque_stride_input); + } +}; + +// This class is used to replace the OpaqueLoopInitNode with a new node while leaving the other nodes +// unchanged. +class ReplaceOpaqueInitNode : public BFSActions { + Node* _new_opaque_init_node; + PhaseIterGVN& _igvn; + + public: + ReplaceOpaqueInitNode(Node* new_opaque_init_node, PhaseIterGVN& igvn) + : _new_opaque_init_node(new_opaque_init_node), + _igvn(igvn) {} + NONCOPYABLE(ReplaceOpaqueInitNode); + + void replace_for(OpaqueTemplateAssertionPredicateNode* opaque_node) { + DataNodeBFS bfs(*this); + bfs.run(opaque_node); + } + + bool should_visit(Node* node) const override { + return TemplateAssertionExpressionNode::is_maybe_in_expression(node); + } + + bool is_target_node(Node* node) const override { + return node->is_OpaqueLoopInit(); + } + + void target_node_action(Node* child, uint i) override { + assert(child->in(i)->is_OpaqueLoopInit(), "must be old OpaqueLoopInit"); + _igvn.replace_input_of(child, i, _new_opaque_init_node); } }; @@ -250,6 +291,13 @@ void TemplateAssertionPredicate::replace_opaque_stride_input(Node* new_stride, P replace_opaque_stride_input.replace_for(opaque_node()); } +// Replace the OpaqueLoopInitNode with 'new_init' and leave the other nodes unchanged. +void TemplateAssertionPredicate::replace_opaque_init_node(Node* new_init, PhaseIterGVN& igvn) const { + DEBUG_ONLY(verify();) + ReplaceOpaqueInitNode replace_opaque_init_node(new_init, igvn); + replace_opaque_init_node.replace_for(opaque_node()); +} + // Create a new Initialized Assertion Predicate from this template at the template success projection. InitializedAssertionPredicate TemplateAssertionPredicate::initialize(PhaseIdealLoop* phase) const { DEBUG_ONLY(verify();) @@ -308,7 +356,8 @@ class OpaqueLoopNodesVerifier : public BFSActions { return node->is_Opaque1(); } - void target_node_action(Node* target_node) override { + void target_node_action(Node* child, uint i) override { + Node* target_node = child->in(i); if (target_node->is_OpaqueLoopInit()) { assert(!_found_init, "should only find one OpaqueLoopInitNode"); _found_init = true; @@ -1094,6 +1143,18 @@ void ClonePredicateToTargetLoop::clone_template_assertion_predicate( _target_loop_predicate_chain.insert_predicate(cloned_template_assertion_predicate); } +// Clones the provided Template Assertion Predicate to the head of the current predicate chain at the target loop and +// replaces the current OpaqueLoopInit with 'new_init'. +// Note: 'new_init' could also have the 'OpaqueLoopInit` as parent node further up. +void ClonePredicateToTargetLoop::clone_template_assertion_predicate_and_replace_init( + const TemplateAssertionPredicate& template_assertion_predicate, Node* new_init) { + TemplateAssertionPredicate cloned_template_assertion_predicate = + template_assertion_predicate.clone_and_replace_init(_old_target_loop_entry, new_init, _target_loop_head->as_CountedLoop(), _phase); + template_assertion_predicate.rewire_loop_data_dependencies(cloned_template_assertion_predicate.tail(), + _node_in_loop_body, _phase); + _target_loop_predicate_chain.insert_predicate(cloned_template_assertion_predicate); +} + CloneUnswitchedLoopPredicatesVisitor::CloneUnswitchedLoopPredicatesVisitor( LoopNode* true_path_loop_head, LoopNode* false_path_loop_head, const NodeInOriginalLoopBody& node_in_true_path_loop_body, const NodeInClonedLoopBody& node_in_false_path_loop_body, @@ -1182,6 +1243,10 @@ void UpdateStrideForAssertionPredicates::connect_initialized_assertion_predicate } } +void UpdateInitForTemplateAssertionPredicates::visit(const TemplateAssertionPredicate& template_assertion_predicate) { + template_assertion_predicate.replace_opaque_init_node(_new_init, _phase->igvn()); +} + // Do the following to find and eliminate useless Parse and Template Assertion Predicates: // 1. Mark all Parse and Template Assertion Predicates "maybe useful". // 2. Walk through the loop tree and iterate over all Predicates above each loop head. All found Parse and Template diff --git a/src/hotspot/share/opto/predicates.hpp b/src/hotspot/share/opto/predicates.hpp index 32b1c1cd3c4..cd0832cc062 100644 --- a/src/hotspot/share/opto/predicates.hpp +++ b/src/hotspot/share/opto/predicates.hpp @@ -438,7 +438,10 @@ class TemplateAssertionPredicate : public Predicate { TemplateAssertionPredicate clone(Node* new_control, CountedLoopNode* new_loop_node, PhaseIdealLoop* phase) const; TemplateAssertionPredicate clone_and_replace_opaque_input(Node* new_control, Node* new_opaque_input, CountedLoopNode* new_loop_node, PhaseIdealLoop* phase) const; + TemplateAssertionPredicate clone_and_replace_init(Node* new_control, Node* new_input, + CountedLoopNode* new_loop_node, PhaseIdealLoop* phase) const; void replace_opaque_stride_input(Node* new_stride, PhaseIterGVN& igvn) const; + void replace_opaque_init_node(Node* new_init, PhaseIterGVN& igvn) const; InitializedAssertionPredicate initialize(PhaseIdealLoop* phase) const; void rewire_loop_data_dependencies(IfTrueNode* target_predicate, const NodeInLoopBody& data_in_loop_body, const PhaseIdealLoop* phase) const; @@ -1228,6 +1231,7 @@ public: } void clone_template_assertion_predicate(const TemplateAssertionPredicate& template_assertion_predicate); + void clone_template_assertion_predicate_and_replace_init(const TemplateAssertionPredicate& template_assertion_predicate, Node* new_init); }; // Visitor to clone Parse and Template Assertion Predicates from a loop to its unswitched true and false path loop. @@ -1300,6 +1304,22 @@ class UpdateStrideForAssertionPredicates : public PredicateVisitor { void visit(const InitializedAssertionPredicate& initialized_assertion_predicate) override; }; +// This visitor replaces the OpaqueLoopInitNode for an Assertion Predicate with the expression passed as input. +class UpdateInitForTemplateAssertionPredicates : public PredicateVisitor { + Node* const _new_init; + PhaseIdealLoop* const _phase; + +public: + UpdateInitForTemplateAssertionPredicates(Node* const new_init, PhaseIdealLoop* phase) + : _new_init(new_init), + _phase(phase) {} + NONCOPYABLE(UpdateInitForTemplateAssertionPredicates); + + using PredicateVisitor::visit; + + void visit(const TemplateAssertionPredicate& template_assertion_predicate) override; +}; + // Eliminate all useless Parse and Template Assertion Predicates. They become useless when they can no longer be found // from a loop head. We mark these useless to clean them up later during IGVN. A Predicate that is marked useless will // no longer be visited by a PredicateVisitor. diff --git a/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate.java b/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate.java new file mode 100644 index 00000000000..334f369cd6b --- /dev/null +++ b/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate.java @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2025 IBM Corporation. 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 8366888 + * @summary C2: incorrect assertion predicate with short running long counted loop + * + * @run main/othervm -XX:-BackgroundCompilation TestShortCountedLoopWithLongRCBadAssertPredicate + * @run main TestShortCountedLoopWithLongRCBadAssertPredicate + */ + +import java.util.Objects; + +public class TestShortCountedLoopWithLongRCBadAssertPredicate { + public static void main(String[] args) { + float[] floatArray = new float[1000]; + for (int i = 0; i < 20_000; i++) { + test1(floatArray, 10000); + test2(floatArray, 10000); + test3(100, 1100, floatArray, 10000); + test4(999, 0, floatArray, 10000); + } + } + + private static float test1(float[] floatArray, long longRange) { + float v = 0; + for (int i = 100; i < 1100; i++) { + v += floatArray[i - 100]; + Objects.checkIndex(i, longRange); + } + return v; + } + + private static float test2(float[] floatArray, long longRange) { + float v = 0; + for (int i = 999; i >= 0; i--) { + v += floatArray[i]; + Objects.checkIndex(i, longRange); + } + return v; + } + + private static float test3(int start, int stop, float[] floatArray, long longRange) { + float v = 0; + for (int i = start; i < stop; i++) { + v += floatArray[i - 100]; + Objects.checkIndex(i, longRange); + } + return v; + } + + private static float test4(int start, int stop, float[] floatArray, long longRange) { + float v = 0; + for (int i = start; i >= stop; i--) { + v += floatArray[i]; + Objects.checkIndex(i, longRange); + } + return v; + } +} diff --git a/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate2.java b/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate2.java new file mode 100644 index 00000000000..13f38f080ed --- /dev/null +++ b/test/hotspot/jtreg/compiler/longcountedloops/TestShortCountedLoopWithLongRCBadAssertPredicate2.java @@ -0,0 +1,54 @@ +/* + * 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 8366888 + * @summary C2: incorrect assertion predicate with short running long counted loop + * + * @run main/othervm -XX:CompileOnly=TestShortCountedLoopWithLongRCBadAssertPredicate2::test -Xbatch TestShortCountedLoopWithLongRCBadAssertPredicate2 + * @run main/othervm TestShortCountedLoopWithLongRCBadAssertPredicate2 + */ + +import java.lang.foreign.MemorySegment; +import java.lang.foreign.ValueLayout; + +public class TestShortCountedLoopWithLongRCBadAssertPredicate2 { + private static MemorySegment ms = MemorySegment.ofArray(new byte[80000]); + private static final ValueLayout.OfByte BYTE = ValueLayout.JAVA_BYTE; + static byte[] bArr = new byte[80000]; + + public static void main(String[] args) { + for (int i = 0; i < 10000; i++) { + test(i); + } + } + + public static void test(int start) { + int end = start + 100; + int i = 0; + while (start < end) { + bArr[i++] = ms.get(BYTE, start++); + } + } +}