From 2527e9e58d770c50e6d807bf1483c6bb07dd3de7 Mon Sep 17 00:00:00 2001 From: Emanuel Peter Date: Thu, 4 Sep 2025 06:53:35 +0000 Subject: [PATCH] 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 | 19 ++- src/hotspot/share/opto/vectorization.hpp | 4 +- src/hotspot/share/opto/vtransform.cpp | 19 ++- src/hotspot/share/opto/vtransform.hpp | 4 +- .../superword/TestAliasingCastP2XCtrl.java | 118 ++++++++++++++++++ 5 files changed, 151 insertions(+), 13 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java diff --git a/src/hotspot/share/opto/vectorization.cpp b/src/hotspot/share/opto/vectorization.cpp index cd5aba6c31d..8e0f0980ff7 100644 --- a/src/hotspot/share/opto/vectorization.cpp +++ b/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); diff --git a/src/hotspot/share/opto/vectorization.hpp b/src/hotspot/share/opto/vectorization.hpp index baca0d5b927..b39e46cbf35 100644 --- a/src/hotspot/share/opto/vectorization.hpp +++ b/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; ) diff --git a/src/hotspot/share/opto/vtransform.cpp b/src/hotspot/share/opto/vtransform.cpp index 2882cc2c1a3..af4cb345e14 100644 --- a/src/hotspot/share/opto/vtransform.cpp +++ b/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 +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); } diff --git a/src/hotspot/share/opto/vtransform.hpp b/src/hotspot/share/opto/vtransform.hpp index 17dd81634ed..60b0b5d4f9d 100644 --- a/src/hotspot/share/opto/vtransform.hpp +++ b/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 + void add_speculative_check(Callback callback); void apply_vectorization() const; }; diff --git a/test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java b/test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java new file mode 100644 index 00000000000..f33a46f4b49 --- /dev/null +++ b/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]); + } + } + } +}