From 9046d7aee3082b6cbf79876efc1c508cb893caad Mon Sep 17 00:00:00 2001 From: Emanuel Peter Date: Tue, 2 Jul 2024 08:20:26 +0000 Subject: [PATCH] 8335390: C2 MergeStores: wrong result with Unsafe Reviewed-by: thartmann, chagedorn, kvn --- src/hotspot/share/opto/memnode.cpp | 7 +- .../jtreg/compiler/c2/TestMergeStores.java | 5 +- .../c2/TestMergeStoresUnsafeArrayPointer.java | 132 ++++++++++++++++++ 3 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/c2/TestMergeStoresUnsafeArrayPointer.java diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index d0b6c59637f..ad3a948793f 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -2984,6 +2984,9 @@ StoreNode* MergePrimitiveArrayStores::run() { type2aelembytes(bt) != _store->memory_size()) { return nullptr; } + if (_store->is_unsafe_access()) { + return nullptr; + } // The _store must be the "last" store in a chain. If we find a use we could merge with // then that use or a store further down is the "last" store. @@ -3017,11 +3020,13 @@ bool MergePrimitiveArrayStores::is_compatible_store(const StoreNode* other_store int opc = _store->Opcode(); assert(opc == Op_StoreB || opc == Op_StoreC || opc == Op_StoreI, "precondition"); assert(_store->adr_type()->isa_aryptr() != nullptr, "must be array store"); + assert(!_store->is_unsafe_access(), "no unsafe accesses"); if (other_store == nullptr || _store->Opcode() != other_store->Opcode() || other_store->adr_type() == nullptr || - other_store->adr_type()->isa_aryptr() == nullptr) { + other_store->adr_type()->isa_aryptr() == nullptr || + other_store->is_unsafe_access()) { return false; } diff --git a/test/hotspot/jtreg/compiler/c2/TestMergeStores.java b/test/hotspot/jtreg/compiler/c2/TestMergeStores.java index 0e86045618b..a94004d8e26 100644 --- a/test/hotspot/jtreg/compiler/c2/TestMergeStores.java +++ b/test/hotspot/jtreg/compiler/c2/TestMergeStores.java @@ -611,8 +611,9 @@ public class TestMergeStores { } @Test - @IR(counts = {IRNode.STORE_L_OF_CLASS, "byte\\\\[int:>=0] \\\\(java/lang/Cloneable,java/io/Serializable\\\\)", "1"}, - applyIf = {"UseUnalignedAccesses", "true"}) + // Disabled by JDK-8335390, to be enabled again by JDK-8335392. + // @IR(counts = {IRNode.STORE_L_OF_CLASS, "byte\\\\[int:>=0] \\\\(java/lang/Cloneable,java/io/Serializable\\\\)", "1"}, + // applyIf = {"UseUnalignedAccesses", "true"}) static Object[] test1f(byte[] a) { UNSAFE.putByte(a, UNSAFE.ARRAY_BYTE_BASE_OFFSET + 0, (byte)0xbe); UNSAFE.putByte(a, UNSAFE.ARRAY_BYTE_BASE_OFFSET + 1, (byte)0xba); diff --git a/test/hotspot/jtreg/compiler/c2/TestMergeStoresUnsafeArrayPointer.java b/test/hotspot/jtreg/compiler/c2/TestMergeStoresUnsafeArrayPointer.java new file mode 100644 index 00000000000..dbfdfe68957 --- /dev/null +++ b/test/hotspot/jtreg/compiler/c2/TestMergeStoresUnsafeArrayPointer.java @@ -0,0 +1,132 @@ +/* + * Copyright (c) 2024, 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 8335390 + * @summary Test merge stores for some Unsafe store address patterns. + * @modules java.base/jdk.internal.misc + * @requires vm.bits == 64 + * @requires os.maxMemory > 8G + * @run main/othervm -XX:CompileCommand=compileonly,compiler.c2.TestMergeStoresUnsafeArrayPointer::test* + * -Xbatch + * -Xmx8g + * compiler.c2.TestMergeStoresUnsafeArrayPointer + * @run main/othervm -Xmx8g + * compiler.c2.TestMergeStoresUnsafeArrayPointer + */ + +package compiler.c2; +import jdk.internal.misc.Unsafe; + +public class TestMergeStoresUnsafeArrayPointer { + static final Unsafe UNSAFE = Unsafe.getUnsafe(); + + // We allocate a big int array of length: + static final int SIZE = (1 << 30) + 100; + + // This gives us a memory region of 4x as many bytes: + static final long BYTE_SIZE = 4L * SIZE; // = 1L << 32 + 400L + + // We set an "anchor" in the middle of this memory region, in bytes: + static final long ANCHOR = BYTE_SIZE / 2; + + static int four = 4; + + public static void main(String[] args) { + System.out.println("Allocate big array of SIZE = " + SIZE); + int[] big = new int[SIZE]; + + // Each test is executed a few times, so that we can see the difference between + // interpreter and compiler. + int errors = 0; + + long val = 0; + System.out.println("test1"); + for (int i = 0; i < 100_000; i++) { + testClear(big); + test1(big, ANCHOR); + long sum = testSum(big); + if (i == 0) { + val = sum; + } else { + if (sum != val) { + System.out.println("ERROR: test1 had wrong value: " + val + " != " + sum); + errors++; + break; + } + } + } + + val = 0; + System.out.println("test2"); + for (int i = 0; i < 100_000; i++) { + testClear(big); + test2(big, ANCHOR); + long sum = testSum(big); + if (i == 0) { + val = sum; + } else { + if (sum != val) { + System.out.println("ERROR: test2 had wrong value: " + val + " != " + sum); + errors++; + break; + } + } + } + + if (errors > 0) { + throw new RuntimeException("ERRORS: " + errors); + } + System.out.println("PASSED"); + } + + // Only clear and sum over relevant parts of array to make the test fast. + static void testClear(int[] a) { + for (int j = 0 ; j < 100; j++) { a[j] = j; } + for (int j = a.length/2 - 100; j < a.length/2 + 100; j++) { a[j] = j; } + for (int j = a.length - 100; j < a.length + 0; j++) { a[j] = j; } + } + + static long testSum(int[] a) { + long sum = 0; + for (int j = 0 ; j < 100; j++) { sum += a[j]; } + for (int j = a.length/2 - 100; j < a.length/2 + 100; j++) { sum += a[j]; } + for (int j = a.length - 100; j < a.length + 0; j++) { sum += a[j]; } + return sum; + } + + // Reference: expected to merge. + static void test1(int[] a, long anchor) { + long base = UNSAFE.ARRAY_INT_BASE_OFFSET + anchor; + UNSAFE.putInt(a, base + 0, 0x42424242); + UNSAFE.putInt(a, base + 4, 0x66666666); + } + + // Test: if MergeStores is applied this can lead to wrong results + static void test2(int[] a, long anchor) { + long base = UNSAFE.ARRAY_INT_BASE_OFFSET + ANCHOR; + UNSAFE.putInt(a, base + 0 + (long)(four + Integer.MAX_VALUE), 0x42424242); + UNSAFE.putInt(a, base + Integer.MAX_VALUE + (long)(four + 4 ), 0x66666666); + } +}