8323274: C2: array load may float above range check

Reviewed-by: epeter, thartmann

src/hotspot/share/opto/arraycopynode.cpp

@@ -152,7 +152,10 @@ int ArrayCopyNode::get_count(PhaseGVN *phase) const {
 }
 
 Node* ArrayCopyNode::load(BarrierSetC2* bs, PhaseGVN *phase, Node*& ctl, MergeMemNode* mem, Node* adr, const TypePtr* adr_type, const Type *type, BasicType bt) {
-  DecoratorSet decorators = C2_READ_ACCESS | C2_CONTROL_DEPENDENT_LOAD | IN_HEAP | C2_ARRAY_COPY;
+  // Pin the load: if this is an array load, it's going to be dependent on a condition that's not a range check for that
+  // access. If that condition is replaced by an identical dominating one, then an unpinned load would risk floating
+  // above runtime checks that guarantee it is within bounds.
+  DecoratorSet decorators = C2_READ_ACCESS | C2_CONTROL_DEPENDENT_LOAD | IN_HEAP | C2_ARRAY_COPY | C2_UNKNOWN_CONTROL_LOAD;
   C2AccessValuePtr addr(adr, adr_type);
   C2OptAccess access(*phase, ctl, mem, decorators, bt, adr->in(AddPNode::Base), addr);
   Node* res = bs->load_at(access, type);

src/hotspot/share/opto/loopnode.hpp

@@ -1744,6 +1744,8 @@ class PhaseIdealLoop : public PhaseTransform {
   void update_addp_chain_base(Node* x, Node* old_base, Node* new_base);
 
   bool can_move_to_inner_loop(Node* n, LoopNode* n_loop, Node* x);
+
+  void pin_array_access_nodes_dependent_on(Node* ctrl);
 };
 
 

src/hotspot/share/opto/loopopts.cpp

@@ -1490,7 +1490,16 @@ void PhaseIdealLoop::split_if_with_blocks_post(Node *n) {
           // Replace the dominated test with an obvious true or false.
           // Place it on the IGVN worklist for later cleanup.
           C->set_major_progress();
-          dominated_by(prevdom->as_IfProj(), n->as_If());
+          // Split if: pin array accesses that are control dependent on a range check and moved to a regular if,
+          // to prevent an array load from floating above its range check. There are three cases:
+          // 1. Move from RangeCheck "a" to RangeCheck "b": don't need to pin. If we ever remove b, then we pin
+          //    all its array accesses at that point.
+          // 2. We move from RangeCheck "a" to regular if "b": need to pin. If we ever remove b, then its array
+          //    accesses would start to float, since we don't pin at that point.
+          // 3. If we move from regular if: don't pin. All array accesses are already assumed to be pinned.
+          bool pin_array_access_nodes =  n->Opcode() == Op_RangeCheck &&
+                                         prevdom->in(0)->Opcode() != Op_RangeCheck;
+          dominated_by(prevdom->as_IfProj(), n->as_If(), false, pin_array_access_nodes);
           DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
           return;
         }
@@ -1664,7 +1673,20 @@ void PhaseIdealLoop::try_sink_out_of_loop(Node* n) {
         // n has a control input inside a loop but get_ctrl() is member of an outer loop. This could happen, for example,
         // for Div nodes inside a loop (control input inside loop) without a use except for an UCT (outside the loop).
         // Rewire control of n to right outside of the loop, regardless if its input(s) are later sunk or not.
-        _igvn.replace_input_of(n, 0, place_outside_loop(n_ctrl, loop_ctrl));
+        Node* maybe_pinned_n = n;
+        Node* outside_ctrl = place_outside_loop(n_ctrl, loop_ctrl);
+        if (n->depends_only_on_test()) {
+          Node* pinned_clone = n->pin_array_access_node();
+          if (pinned_clone != nullptr) {
+            // Pin array access nodes: if this is an array load, it's going to be dependent on a condition that's not a
+            // range check for that access. If that condition is replaced by an identical dominating one, then an
+            // unpinned load would risk floating above its range check.
+            register_new_node(pinned_clone, n_ctrl);
+            maybe_pinned_n = pinned_clone;
+            _igvn.replace_node(n, pinned_clone);
+          }
+        }
+        _igvn.replace_input_of(maybe_pinned_n, 0, outside_ctrl);
       }
     }
     if (n_loop != _ltree_root && n->outcnt() > 1) {
@@ -1678,7 +1700,16 @@ void PhaseIdealLoop::try_sink_out_of_loop(Node* n) {
         for (DUIterator_Last jmin, j = n->last_outs(jmin); j >= jmin;) {
           Node* u = n->last_out(j); // Clone private computation per use
           _igvn.rehash_node_delayed(u);
-          Node* x = n->clone(); // Clone computation
+          Node* x = nullptr;
+          if (n->depends_only_on_test()) {
+            // Pin array access nodes: if this is an array load, it's going to be dependent on a condition that's not a
+            // range check for that access. If that condition is replaced by an identical dominating one, then an
+            // unpinned load would risk floating above its range check.
+            x = n->pin_array_access_node();
+          }
+          if (x == nullptr) {
+            x = n->clone();
+          }
           Node* x_ctrl = nullptr;
           if (u->is_Phi()) {
             // Replace all uses of normal nodes.  Replace Phi uses
@@ -2228,6 +2259,20 @@ void PhaseIdealLoop::clone_loop_handle_data_uses(Node* old, Node_List &old_new,
       // We notify all uses of old, including use, and the indirect uses,
       // that may now be optimized because we have replaced old with phi.
       _igvn.add_users_to_worklist(old);
+      if (idx == 0 &&
+          use->depends_only_on_test()) {
+        Node* pinned_clone = use->pin_array_access_node();
+        if (pinned_clone != nullptr) {
+          // Pin array access nodes: control is updated here to a region. If, after some transformations, only one path
+          // into the region is left, an array load could become dependent on a condition that's not a range check for
+          // that access. If that condition is replaced by an identical dominating one, then an unpinned load would risk
+          // floating above its range check.
+          pinned_clone->set_req(0, phi);
+          register_new_node(pinned_clone, get_ctrl(use));
+          _igvn.replace_node(use, pinned_clone);
+          continue;
+        }
+      }
       _igvn.replace_input_of(use, idx, phi);
       if( use->_idx >= new_counter ) { // If updating new phis
         // Not needed for correctness, but prevents a weak assert
@@ -3863,6 +3908,19 @@ bool PhaseIdealLoop::partial_peel( IdealLoopTree *loop, Node_List &old_new ) {
     if (!n->is_CFG()           && n->in(0) != nullptr        &&
         not_peel.test(n->_idx) && peel.test(n->in(0)->_idx)) {
       Node* n_clone = old_new[n->_idx];
+      if (n_clone->depends_only_on_test()) {
+        // Pin array access nodes: control is updated here to the loop head. If, after some transformations, the
+        // backedge is removed, an array load could become dependent on a condition that's not a range check for that
+        // access. If that condition is replaced by an identical dominating one, then an unpinned load would risk
+        // floating above its range check.
+        Node* pinned_clone = n_clone->pin_array_access_node();
+        if (pinned_clone != nullptr) {
+          register_new_node(pinned_clone, get_ctrl(n_clone));
+          old_new.map(n->_idx, pinned_clone);
+          _igvn.replace_node(n_clone, pinned_clone);
+          n_clone = pinned_clone;
+        }
+      }
       _igvn.replace_input_of(n_clone, 0, new_head_clone);
     }
   }

src/hotspot/share/opto/split_if.cpp

@@ -727,6 +727,14 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio
   } // End of while merge point has phis
 
   _igvn.remove_dead_node(region);
+  if (iff->Opcode() == Op_RangeCheck) {
+    // Pin array access nodes: control is updated here to a region. If, after some transformations, only one path
+    // into the region is left, an array load could become dependent on a condition that's not a range check for
+    // that access. If that condition is replaced by an identical dominating one, then an unpinned load would risk
+    // floating above its range check.
+    pin_array_access_nodes_dependent_on(new_true);
+    pin_array_access_nodes_dependent_on(new_false);
+  }
 
   if (new_false_region != nullptr) {
     *new_false_region = new_false;
@@ -737,3 +745,18 @@ void PhaseIdealLoop::do_split_if(Node* iff, RegionNode** new_false_region, Regio
 
   DEBUG_ONLY( if (VerifyLoopOptimizations) { verify(); } );
 }
+
+void PhaseIdealLoop::pin_array_access_nodes_dependent_on(Node* ctrl) {
+  for (DUIterator i = ctrl->outs(); ctrl->has_out(i); i++) {
+    Node* use = ctrl->out(i);
+    if (!use->depends_only_on_test()) {
+      continue;
+    }
+    Node* pinned_clone = use->pin_array_access_node();
+    if (pinned_clone != nullptr) {
+      register_new_node(pinned_clone, get_ctrl(use));
+      _igvn.replace_node(use, pinned_clone);
+      --i;
+    }
+  }
+}

test/hotspot/jtreg/compiler/rangechecks/TestArrayAccessAboveRCAfterPartialPeeling.java

@@ -0,0 +1,130 @@
+/*
+ * 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 8323274
+ * @summary partial peeling loop can cause an array load to become dependent on a test other than its range check
+ * @run main/othervm -XX:-UseOnStackReplacement -XX:-TieredCompilation -XX:-BackgroundCompilation TestArrayAccessAboveRCAfterPartialPeeling
+ */
+
+public class TestArrayAccessAboveRCAfterPartialPeeling {
+    private static volatile int volatileField;
+
+    public static void main(String[] args) {
+        int[] array = new int[100];
+        for (int i = 0; i < 20_000; i++) {
+            test(array, 2, true, 1);
+            test(array, 2, false, 1);
+            inlined(array, 2, 42, true, 42, 1, 1);
+            inlined(array, 2, 42, false, 42, 1, 1);
+        }
+        try {
+            test(array, 2, true, -1);
+        } catch (ArrayIndexOutOfBoundsException arrayIndexOutOfBoundsException) {
+        }
+    }
+
+    private static int test(int[] array, int k, boolean flag, int j) {
+        int l;
+        for (l = 1; l < 2; l *= 2) {
+
+        }
+        int m;
+        for (m = 0; m < 42; m += l) {
+
+        }
+        int n;
+        for (n = 0; n < 10; n += m/42) {
+
+        }
+        return inlined(array, k, l, flag, m, n/10, j);
+    }
+
+    private static int inlined(int[] array, int k, int l, boolean flag, int m, int n, int j) {
+        if (array == null) {
+        }
+        int[] otherArray = new int[100];
+        int i = 0;
+        int v = 0;
+        if (k == m) {
+        }
+
+        if (flag) {
+            v += array[j];
+            v += otherArray[i];
+
+            for (; ; ) {
+                synchronized (new Object()) {
+                }
+                if (j >= 100) {
+                    break;
+                }
+                if (k == 42) {
+                }
+                v += array[j];
+                v += otherArray[i];
+                if (i >= n) {
+                    otherArray[i] = v;
+                }
+                v += array[j];
+                if (l == 2) {
+                    break;
+                }
+                i++;
+                j *= 2;
+                volatileField = 42;
+                k = 2;
+                l = 42;
+            }
+        } else {
+            v += array[j];
+            v += otherArray[i];
+
+            for (; ; ) {
+                synchronized (new Object()) {
+                }
+                if (j >= 100) {
+                    break;
+                }
+                if (k == 42) {
+                }
+                v += array[j];
+                v += otherArray[i];
+                if (i >= n) {
+                    otherArray[i] = v;
+                }
+                v += array[j];
+                if (l == 2) {
+                    break;
+                }
+                i++;
+                j *= 2;
+                volatileField = 42;
+                k = 2;
+                l = 42;
+            }
+        }
+        return v;
+    }
+}