8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops

Reviewed-by: thartmann, chagedorn, mhaessig

src/hotspot/share/opto/vectorization.cpp

@@ -934,7 +934,7 @@ BoolNode* make_a_plus_b_leq_c(Node* a, Node* b, Node* c, PhaseIdealLoop* phase)
   return bol;
 }
 
-BoolNode* VPointer::make_speculative_aliasing_check_with(const VPointer& other) const {
+BoolNode* VPointer::make_speculative_aliasing_check_with(const VPointer& other, Node* ctrl) const {
   // Ensure iv_scale1 <= iv_scale2.
   const VPointer& vp1 = (this->iv_scale() <= other.iv_scale()) ? *this : other;
   const VPointer& vp2 = (this->iv_scale() <= other.iv_scale()) ? other :*this ;
@@ -971,8 +971,8 @@ BoolNode* VPointer::make_speculative_aliasing_check_with(const VPointer& other)
   Node* main_init = new ConvL2INode(main_initL);
   phase->register_new_node_with_ctrl_of(main_init, pre_init);
 
-  Node* p1_init = vp1.make_pointer_expression(main_init);
-  Node* p2_init = vp2.make_pointer_expression(main_init);
+  Node* p1_init = vp1.make_pointer_expression(main_init, ctrl);
+  Node* p2_init = vp2.make_pointer_expression(main_init, ctrl);
   Node* size1 = igvn.longcon(vp1.size());
   Node* size2 = igvn.longcon(vp2.size());
 
@@ -1092,13 +1092,18 @@ BoolNode* VPointer::make_speculative_aliasing_check_with(const VPointer& other)
   return bol;
 }
 
-Node* VPointer::make_pointer_expression(Node* iv_value) const {
+// Creates the long pointer expression, evaluated with iv = iv_value.
+// Since we are casting pointers to long with CastP2X, we must be careful
+// that the values do not cross SafePoints, where the oop could be moved
+// by GC, and the already cast value would not be updated, as it is not in
+// the oop-map. For this, we must set a ctrl that is late enough, so that we
+// cannot cross a SafePoint.
+Node* VPointer::make_pointer_expression(Node* iv_value, Node* ctrl) const {
   assert(is_valid(), "must be valid");
 
   PhaseIdealLoop* phase = _vloop.phase();
   PhaseIterGVN& igvn = phase->igvn();
   Node* iv = _vloop.iv();
-  Node* ctrl = phase->get_ctrl(iv_value);
 
   auto maybe_add = [&] (Node* n1, Node* n2, BasicType bt) {
     if (n1 == nullptr) { return n2; }
@@ -1120,7 +1125,9 @@ Node* VPointer::make_pointer_expression(Node* iv_value) const {
       Node* scaleL = igvn.longcon(s.scaleL().value());
       Node* variable = (s.variable() == iv) ? iv_value : s.variable();
       if (variable->bottom_type()->isa_ptr() != nullptr) {
-        variable = new CastP2XNode(nullptr, variable);
+        // Use a ctrl that is late enough, so that we do not
+        // evaluate the cast before a SafePoint.
+        variable = new CastP2XNode(ctrl, variable);
         phase->register_new_node(variable, ctrl);
       }
       node = new MulLNode(scaleL, variable);

src/hotspot/share/opto/vectorization.hpp

@@ -1013,8 +1013,8 @@ public:
   }
 
   bool can_make_speculative_aliasing_check_with(const VPointer& other) const;
-  Node* make_pointer_expression(Node* iv_value) const;
-  BoolNode* make_speculative_aliasing_check_with(const VPointer& other) const;
+  Node* make_pointer_expression(Node* iv_value, Node* ctrl) const;
+  BoolNode* make_speculative_aliasing_check_with(const VPointer& other, Node* ctrl) const;
 
   NOT_PRODUCT( void print_on(outputStream* st, bool end_with_cr = true) const; )
 

src/hotspot/share/opto/vtransform.cpp

@@ -217,7 +217,7 @@ void VTransform::add_speculative_alignment_check(Node* node, juint alignment) {
   TRACE_SPECULATIVE_ALIGNMENT_CHECK(cmp_alignment);
   TRACE_SPECULATIVE_ALIGNMENT_CHECK(bol_alignment);
 
-  add_speculative_check(bol_alignment);
+  add_speculative_check([&] (Node* ctrl) { return bol_alignment; });
 }
 
 class VPointerWeakAliasingPair : public StackObj {
@@ -389,8 +389,9 @@ void VTransform::apply_speculative_aliasing_runtime_checks() {
       }
 #endif
 
-      BoolNode* bol = vp1_union.make_speculative_aliasing_check_with(vp2_union);
-      add_speculative_check(bol);
+      add_speculative_check([&] (Node* ctrl) {
+        return vp1_union.make_speculative_aliasing_check_with(vp2_union, ctrl);
+      });
 
       group_start = group_end;
     }
@@ -420,7 +421,13 @@ void VTransform::apply_speculative_aliasing_runtime_checks() {
 //       Multiversioning takes more compile time and code cache, but it also
 //       produces fast code for when the runtime check passes (vectorized) and
 //       when it fails (scalar performance).
-void VTransform::add_speculative_check(BoolNode* bol) {
+//
+// Callback:
+//   In some cases, we require the ctrl just before the check iff_speculate to
+//   generate the values required in the check. We pass this ctrl into the
+//   callback, which is expected to produce the check, i.e. a BoolNode.
+template<typename Callback>
+void VTransform::add_speculative_check(Callback callback) {
   assert(_vloop.are_speculative_checks_possible(), "otherwise we cannot make speculative assumptions");
   ParsePredicateSuccessProj* parse_predicate_proj = _vloop.auto_vectorization_parse_predicate_proj();
   IfTrueNode* new_check_proj = nullptr;
@@ -432,6 +439,10 @@ void VTransform::add_speculative_check(BoolNode* bol) {
     new_check_proj = phase()->create_new_if_for_multiversion(_vloop.multiversioning_fast_proj());
   }
   Node* iff_speculate = new_check_proj->in(0);
+
+  // Create the check, given the ctrl just before the iff.
+  BoolNode* bol = callback(iff_speculate->in(0));
+
   igvn().replace_input_of(iff_speculate, 1, bol);
   TRACE_SPECULATIVE_ALIGNMENT_CHECK(iff_speculate);
 }

src/hotspot/share/opto/vtransform.hpp

@@ -258,7 +258,9 @@ private:
   void apply_speculative_alignment_runtime_checks();
   void apply_speculative_aliasing_runtime_checks();
   void add_speculative_alignment_check(Node* node, juint alignment);
-  void add_speculative_check(BoolNode* bol);
+
+  template<typename Callback>
+  void add_speculative_check(Callback callback);
 
   void apply_vectorization() const;
 };

test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java

@@ -0,0 +1,118 @@
+/*
+ * 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 id=all-flags
+ * @bug 8366490
+ * @summary Test that we set the ctrl of CastP2X when generating
+ *          the aliasing runtime check, preventing the CastP2X
+ *          from floating over a SafePoint that could move the oop,
+ *          and render the cast value stale.
+ * @requires vm.gc == "G1" | vm.gc == "null"
+ * @run main/othervm
+ *      -XX:+IgnoreUnrecognizedVMOptions
+ *      -XX:CompileCommand=compileonly,*TestAliasingCastP2XCtrl::test
+ *      -XX:CompileCommand=dontinline,*TestAliasingCastP2XCtrl::allocateArrays
+ *      -XX:-TieredCompilation
+ *      -Xbatch
+ *      -XX:+UseG1GC
+ *      -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM
+ *      compiler.loopopts.superword.TestAliasingCastP2XCtrl
+ */
+
+/*
+ * @test id=fewer-flags
+ * @bug 8366490
+ * @run main/othervm
+ *      -XX:+IgnoreUnrecognizedVMOptions
+ *      -XX:CompileCommand=compileonly,*TestAliasingCastP2XCtrl::test
+ *      -XX:CompileCommand=dontinline,*TestAliasingCastP2XCtrl::allocateArrays
+ *      -Xbatch
+ *      -XX:+UnlockDiagnosticVMOptions -XX:+StressGCM
+ *      compiler.loopopts.superword.TestAliasingCastP2XCtrl
+ */
+
+/*
+ * @test id=vanilla
+ * @bug 8366490
+ * @run main compiler.loopopts.superword.TestAliasingCastP2XCtrl
+ */
+
+package compiler.loopopts.superword;
+
+public class TestAliasingCastP2XCtrl {
+    static final int N = 400;
+    static boolean flag = false;
+
+    static void allocateArrays() {
+        for (int i = 0; 200_000 > i; ++i) {
+            int[] a = new int[N];
+        }
+        // Makes GC more likely.
+        // Without it I could not reproduce it on slowdebug,
+        // but only with fastdebug.
+        if (flag) { System.gc(); }
+        flag = !flag;
+    }
+
+    static int[] test() {
+        int a[] = new int[N];
+        // We must make sure that no CastP2X happens before
+        // the call below, otherwise we may have an old oop.
+        allocateArrays();
+        // The CastP2X for the aliasing runtime check should
+        // only be emitted after the call, to ensure we only
+        // deal with oops that are updated if there is a GC
+        // that could move our allocated array.
+
+        // Not fully sure why we need the outer loop, but maybe
+        // it is needed so that a part of the check is hoisted,
+        // and the floats up, over the call if we do not set
+        // the ctrl.
+        for (int k = 0; k < 500; k++) {
+            for (int i = 1; i < 69; i++) {
+                // Aliasing references -> needs runtime check,
+                // should always fail.
+                a[i] =  14;
+                a[4] -= 14;
+                // The range computation for the constant access
+                // produces a shape:
+                //   AddL(CastP2X(a), 0x20)
+                // And this shape only depends on a, so it could
+                // easily float above the call to allocateArrays
+                // if we do not set a ctrl that prevents that.
+            }
+        }
+        return a;
+    }
+
+    public static void main(String[] args) {
+        int[] gold = test();
+        for (int r = 0; r < 20; r++) {
+            int[] a = test();
+            if (a[4] != gold[4]) {
+                throw new RuntimeException("wrong value " + gold[4] + " " + a[4]);
+            }
+        }
+    }
+}