From cfabcbf85837e97bdd6c9e4e06e875ecbaa70084 Mon Sep 17 00:00:00 2001 From: Vladimir Kozlov Date: Tue, 3 Oct 2023 18:18:26 +0000 Subject: [PATCH] 8317121: vector_masked_load instruction is moved too early after JDK-8286941 Co-authored-by: Vladimir Ivanov Co-authored-by: Tobias Hartmann Co-authored-by: Dean Long Reviewed-by: thartmann, vlivanov, epeter --- src/hotspot/share/opto/memnode.cpp | 9 +- .../vectorization/TestMaskedVectors.java | 118 ++++++++++++++++++ 2 files changed, 125 insertions(+), 2 deletions(-) create mode 100644 test/hotspot/jtreg/compiler/vectorization/TestMaskedVectors.java diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp index ab72979ffcb..c402a240acb 100644 --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -593,8 +593,13 @@ Node* LoadNode::find_previous_arraycopy(PhaseValues* phase, Node* ld_alloc, Node Node* dest = ac->in(ArrayCopyNode::Dest); if (dest == ld_base) { - const TypeX *ld_offs_t = phase->type(ld_offs)->isa_intptr_t(); - if (ac->modifies(ld_offs_t->_lo, ld_offs_t->_hi, phase, can_see_stored_value)) { + const TypeX* ld_offs_t = phase->type(ld_offs)->isa_intptr_t(); + assert(!ld_offs_t->empty(), "dead reference should be checked already"); + // Take into account vector or unsafe access size + jlong ld_size_in_bytes = (jlong)memory_size(); + jlong offset_hi = ld_offs_t->_hi + ld_size_in_bytes - 1; + offset_hi = MIN2(offset_hi, (jlong)(TypeX::MAX->_hi)); // Take care for overflow in 32-bit VM + if (ac->modifies(ld_offs_t->_lo, (intptr_t)offset_hi, phase, can_see_stored_value)) { return ac; } if (!can_see_stored_value) { diff --git a/test/hotspot/jtreg/compiler/vectorization/TestMaskedVectors.java b/test/hotspot/jtreg/compiler/vectorization/TestMaskedVectors.java new file mode 100644 index 00000000000..c106a9bc69c --- /dev/null +++ b/test/hotspot/jtreg/compiler/vectorization/TestMaskedVectors.java @@ -0,0 +1,118 @@ +/* + * Copyright (c) 2023, 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 8317121 + * @summary Test masked vectors and unsafe access to memory modified by arraycopy + * @requires vm.compiler2.enabled + * @modules java.base/jdk.internal.misc + * @library /test/lib / + * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -Xbatch -XX:CompileCommand=quiet -XX:CompileCommand=compileonly,TestMaskedVectors::test* -XX:+StressLCM -XX:+StressGCM -XX:StressSeed=2210259638 TestMaskedVectors + * @run main/othervm -XX:+IgnoreUnrecognizedVMOptions -XX:+UnlockDiagnosticVMOptions -XX:-TieredCompilation -Xbatch -XX:CompileCommand=quiet -XX:CompileCommand=compileonly,TestMaskedVectors::test* -XX:+StressLCM -XX:+StressGCM TestMaskedVectors + */ + +import java.lang.reflect.*; +import java.util.*; + +import jdk.internal.misc.Unsafe; + +public class TestMaskedVectors { + + private static Unsafe UNSAFE = Unsafe.getUnsafe(); + private static final long BASE_OFFSET = UNSAFE.arrayBaseOffset(byte[].class); + + static void testLoadVectorMasked(byte[] src, byte[] dst, int len) { + byte[] tmp = new byte[64]; + + // (3) The LoadVectorMasked is found to be dependent on below arraycopy and + // therefore scheduled just below it. As a result, the LoadVectorMasked misses the + // updated elements at index 16..48 and dst will contain incorrect values. + System.arraycopy(src, 0, tmp, 0, 16); + + // (2) The LoadVectorMasked is incorrectly found to be independent of this arraycopy + // because the LoadVectorMasked has offset 0 whereas the arraycopy writes offset >= 16. + // The problem is that MemNode::find_previous_store() -> LoadNode::find_previous_arraycopy() + // -> ArrayCopyNode::modifies does not account for the size of the load. + System.arraycopy(src, 0, tmp, 16, 48); + + // (1) The following arraycopy is expanded into a LoadVectorMasked and a + // StoreVectorMasked in PhaseMacroExpand::generate_partial_inlining_block(). + System.arraycopy(tmp, 0, dst, 0, len); + } + + static long testUnsafeGetLong(byte[] src) { + byte[] tmp = new byte[16]; + + // (3) The unsafe load is found to be dependent on below arraycopy and + // therefore scheduled just below it. As a result, the unsafe load misses the + // updated elements at index 1..16 and therefore returns an incorrect result. + System.arraycopy(src, 0, tmp, 0, 16); + + // (2) The unsafe load is incorrectly found to be independent of this arraycopy + // because the load has offset 0 in 'tmp' whereas the arraycopy writes offsets >= 1. + // The problem is that MemNode::find_previous_store() -> LoadNode::find_previous_arraycopy() + // -> ArrayCopyNode::modifies does not account for the size of the load. + System.arraycopy(src, 0, tmp, 1, 15); + + // (1) Below unsafe load reads the first 8 (byte) array elements. + return UNSAFE.getLong(tmp, BASE_OFFSET); + } + + public static void main(String[] args) { + // Initialize src array with increasing byte values + byte[] src = new byte[64]; + for (byte i = 0; i < src.length; ++i) { + src[i] = (byte)i; + } + + // Compute expected outputs once + byte[] golden1 = new byte[64]; + testLoadVectorMasked(src, golden1, 64); + + long golden2 = testUnsafeGetLong(src); + + // Trigger compilation of test methods and verify the results + for (int i = 0; i < 50_000; ++i) { + int len = i % 32; + byte[] dst = new byte[len]; + testLoadVectorMasked(src, dst, len); + + boolean error = false; + for (int j = 0; j < dst.length; ++j) { + if (dst[j] != golden1[j]) { + System.out.println("Incorrect value of element " + j + ": Expected " + golden1[j] + " but got " + dst[j]); + error = true; + } + } + if (error) { + throw new RuntimeException("Test LoadVectorMasked failed"); + } + + long res = testUnsafeGetLong(src); + if (res != golden2) { + throw new RuntimeException("Incorrect result in test UnsafeGetLong: Expected " + golden2 + " but got " + res); + } + } + } +}