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

Reviewed-by: kvn
Backport-of: 8a83dc213ac630ec79d62637133fe7aa102a27a3

Author: chhagedorn

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
@@ -1428,11 +1428,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) {
@@ -1445,21 +1447,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).
@@ -1734,10 +1728,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
@@ -1746,7 +1745,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,
@@ -1768,6 +1767,47 @@ void PhaseIdealLoop::create_assertion_predicates_at_loop(CountedLoopNode* source
     set_idom(target_outer_loop_head, last_created_predicate_success_proj, dom_depth(target_outer_loop_head));
   }
 }
+
+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) {

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
@@ -950,12 +950,20 @@ class PhaseIdealLoop : public PhaseTransform {
                                                        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();

src/hotspot/share/opto/predicates.cpp

@@ -135,7 +135,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
     }

src/hotspot/share/opto/predicates.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, 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
@@ -249,7 +249,7 @@ class PredicateVisitor : 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).
@@ -952,13 +952,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;
   }
 };
 
@@ -973,7 +1012,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;
   }
 };

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