diff --git a/src/hotspot/share/opto/arraycopynode.cpp b/src/hotspot/share/opto/arraycopynode.cpp index dab0ca98911..2f64482f55b 100644 --- a/src/hotspot/share/opto/arraycopynode.cpp +++ b/src/hotspot/share/opto/arraycopynode.cpp @@ -258,6 +258,19 @@ Node* ArrayCopyNode::try_clone_instance(PhaseGVN *phase, bool can_reshape, int c return mem; } +// We may have narrowed the type of base because this runs with PhaseIterGVN::_delay_transform true, explicitly +// update the type of the AddP so it's consistent with its base and load() picks the right memory slice. +Node* ArrayCopyNode::make_and_transform_addp(PhaseGVN* phase, Node* base, Node* offset) { + return make_and_transform_addp(phase, base, base, offset); +} + +Node* ArrayCopyNode::make_and_transform_addp(PhaseGVN* phase, Node* base, Node* ptr, Node* offset) { + assert(phase->is_IterGVN() == nullptr || phase->is_IterGVN()->delay_transform(), "helper method when delay transform is set"); + Node* addp = phase->transform(AddPNode::make_with_base(base, ptr, offset)); + phase->set_type(addp, addp->Value(phase)); + return addp; +} + bool ArrayCopyNode::prepare_array_copy(PhaseGVN *phase, bool can_reshape, Node*& adr_src, Node*& base_src, @@ -332,12 +345,11 @@ bool ArrayCopyNode::prepare_array_copy(PhaseGVN *phase, bool can_reshape, Node* dest_scale = phase->transform(new LShiftXNode(dest_offset, phase->intcon(shift))); - adr_src = phase->transform(AddPNode::make_with_base(base_src, src_scale)); - adr_dest = phase->transform(AddPNode::make_with_base(base_dest, dest_scale)); - - adr_src = phase->transform(AddPNode::make_with_base(base_src, adr_src, phase->MakeConX(header))); - adr_dest = phase->transform(AddPNode::make_with_base(base_dest, adr_dest, phase->MakeConX(header))); + adr_src = make_and_transform_addp(phase, base_src, src_scale); + adr_dest = make_and_transform_addp(phase, base_dest, dest_scale); + adr_src = make_and_transform_addp(phase, base_src, adr_src, phase->MakeConX(header)); + adr_dest = make_and_transform_addp(phase, base_dest, adr_dest, phase->MakeConX(header)); copy_type = dest_elem; } else { assert(ary_src != nullptr, "should be a clone"); @@ -355,8 +367,8 @@ bool ArrayCopyNode::prepare_array_copy(PhaseGVN *phase, bool can_reshape, return false; } - adr_src = phase->transform(AddPNode::make_with_base(base_src, src_offset)); - adr_dest = phase->transform(AddPNode::make_with_base(base_dest, dest_offset)); + adr_src = make_and_transform_addp(phase, base_src, src_offset); + adr_dest = make_and_transform_addp(phase, base_dest, dest_offset); // The address is offsetted to an aligned address where a raw copy would start. // If the clone copy is decomposed into load-stores - the address is adjusted to @@ -366,8 +378,8 @@ bool ArrayCopyNode::prepare_array_copy(PhaseGVN *phase, bool can_reshape, int diff = arrayOopDesc::base_offset_in_bytes(elem) - offset; assert(diff >= 0, "clone should not start after 1st array element"); if (diff > 0) { - adr_src = phase->transform(AddPNode::make_with_base(base_src, adr_src, phase->MakeConX(diff))); - adr_dest = phase->transform(AddPNode::make_with_base(base_dest, adr_dest, phase->MakeConX(diff))); + adr_src = make_and_transform_addp(phase, base_src, adr_src, phase->MakeConX(diff)); + adr_dest = make_and_transform_addp(phase, base_dest, adr_dest, phase->MakeConX(diff)); } copy_type = elem; value_type = ary_src->elem(); @@ -429,12 +441,8 @@ Node* ArrayCopyNode::array_copy_forward(PhaseGVN *phase, store(bs, phase, forward_ctl, mm, adr_dest, atp_dest, v, value_type, copy_type); for (int i = 1; i < count; i++) { Node* off = phase->MakeConX(type2aelembytes(copy_type) * i); - Node* next_src = phase->transform(AddPNode::make_with_base(base_src,adr_src,off)); - // We may have narrowed the type of next_src right before calling this method but because this runs with - // PhaseIterGVN::_delay_transform true, explicitly update the type of the AddP so it's consistent with its - // base and load() picks the right memory slice. - phase->set_type(next_src, next_src->Value(phase)); - Node* next_dest = phase->transform(AddPNode::make_with_base(base_dest,adr_dest,off)); + Node* next_src = make_and_transform_addp(phase, base_src,adr_src,off); + Node* next_dest = make_and_transform_addp(phase, base_dest,adr_dest,off); // Same as above phase->set_type(next_dest, next_dest->Value(phase)); v = load(bs, phase, forward_ctl, mm, next_src, atp_src, value_type, copy_type); @@ -473,13 +481,8 @@ Node* ArrayCopyNode::array_copy_backward(PhaseGVN *phase, if (count > 0) { for (int i = count-1; i >= 1; i--) { Node* off = phase->MakeConX(type2aelembytes(copy_type) * i); - Node* next_src = phase->transform(AddPNode::make_with_base(base_src,adr_src,off)); - // We may have narrowed the type of next_src right before calling this method but because this runs with - // PhaseIterGVN::_delay_transform true, explicitly update the type of the AddP so it's consistent with its - // base and store() picks the right memory slice. - phase->set_type(next_src, next_src->Value(phase)); - Node* next_dest = phase->transform(AddPNode::make_with_base(base_dest,adr_dest,off)); - phase->set_type(next_dest, next_dest->Value(phase)); + Node* next_src = make_and_transform_addp(phase, base_src,adr_src,off); + Node* next_dest = make_and_transform_addp(phase, base_dest,adr_dest,off); Node* v = load(bs, phase, backward_ctl, mm, next_src, atp_src, value_type, copy_type); store(bs, phase, backward_ctl, mm, next_dest, atp_dest, v, value_type, copy_type); } @@ -618,21 +621,31 @@ Node *ArrayCopyNode::Ideal(PhaseGVN *phase, bool can_reshape) { phase->set_type(src, phase->type(src)->join_speculative(atp_src)); phase->set_type(dest, phase->type(dest)->join_speculative(atp_dest)); + // Control flow is going to be created, it's easier to do with _delay_transform set to true. + + // prepare_array_copy() doesn't build control flow, but it creates AddP nodes. The src/dest type possibly gets + // narrowed above. If a newly created AddP node is commoned with a pre-existing one, then the type narrowing is lost. + // Setting _delay_transform before prepare_array_copy() guarantees this doesn't happen. + if (can_reshape) { + assert(!phase->is_IterGVN()->delay_transform(), "cannot delay transforms"); + phase->is_IterGVN()->set_delay_transform(true); + } + if (!prepare_array_copy(phase, can_reshape, adr_src, base_src, adr_dest, base_dest, copy_type, value_type, disjoint_bases)) { assert(adr_src == nullptr, "no node can be left behind"); assert(adr_dest == nullptr, "no node can be left behind"); + if (can_reshape) { + assert(phase->is_IterGVN()->delay_transform(), "cannot delay transforms"); + phase->is_IterGVN()->set_delay_transform(false); + } + return nullptr; } Node* in_mem = in(TypeFunc::Memory); - if (can_reshape) { - assert(!phase->is_IterGVN()->delay_transform(), "cannot delay transforms"); - phase->is_IterGVN()->set_delay_transform(true); - } - Node* backward_ctl = phase->C->top(); Node* forward_ctl = phase->C->top(); array_copy_test_overlap(phase, can_reshape, disjoint_bases, count, forward_ctl, backward_ctl); diff --git a/src/hotspot/share/opto/arraycopynode.hpp b/src/hotspot/share/opto/arraycopynode.hpp index 483a3a86267..aa62ee05cd0 100644 --- a/src/hotspot/share/opto/arraycopynode.hpp +++ b/src/hotspot/share/opto/arraycopynode.hpp @@ -104,6 +104,10 @@ private: static const TypePtr* get_address_type(PhaseGVN* phase, const TypePtr* atp, Node* n); Node* try_clone_instance(PhaseGVN *phase, bool can_reshape, int count); + + Node* make_and_transform_addp(PhaseGVN* phase, Node* base, Node* offset); + Node* make_and_transform_addp(PhaseGVN* phase, Node* base, Node* ptr, Node* offset); + bool prepare_array_copy(PhaseGVN *phase, bool can_reshape, Node*& adr_src, Node*& base_src, Node*& adr_dest, Node*& base_dest, BasicType& copy_type, const Type*& value_type, bool& disjoint_bases); diff --git a/test/hotspot/jtreg/compiler/arraycopy/TestACNonEscapingSrcBadAddPBaseType.java b/test/hotspot/jtreg/compiler/arraycopy/TestACNonEscapingSrcBadAddPBaseType.java new file mode 100644 index 00000000000..251c053ff5d --- /dev/null +++ b/test/hotspot/jtreg/compiler/arraycopy/TestACNonEscapingSrcBadAddPBaseType.java @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2026 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 8380158 + * @summary C2: compiler/c2/TestGVNCrash.java asserts with adr and adr_type must agree + * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation + * -XX:CompileOnly=compiler.arraycopy.TestACNonEscapingSrcBadAddPBaseType::test1 + * -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN -XX:StressSeed=946074051 ${test.main.class} + * @run main/othervm -XX:-TieredCompilation -XX:-UseOnStackReplacement -XX:-BackgroundCompilation + * -XX:CompileOnly=compiler.arraycopy.TestACNonEscapingSrcBadAddPBaseType::test1 + * -XX:+UnlockDiagnosticVMOptions -XX:+StressIGVN ${test.main.class} + * @run main ${test.main.class} + */ + +package compiler.arraycopy; + +public class TestACNonEscapingSrcBadAddPBaseType { + private static volatile int volatileField; + + public static void main(String[] args) { + int[] dst = new int[2]; + for (int i = 0; i < 20_000; i++) { + test1(dst); + } + } + + private static void test1(int[] dst) { + int[] array = new int[2]; + A a = new A(array); + volatileField = 42; + int[] src = a.src; + System.arraycopy(src, 0, dst, 0, src.length); + System.arraycopy(src, 0, dst, 0, src.length); + } + + private static class A { + private final int[] src; + + public A(int[] src) { + this.src = src; + } + } +}