From e67550cfec4dbd1c8c2c9869dda34fa09a5c274b Mon Sep 17 00:00:00 2001 From: Claes Redestad Date: Tue, 24 Oct 2023 13:32:41 +0000 Subject: [PATCH] 8318509: x86 count_positives intrinsic broken for -XX:AVX3Threshold=0 Reviewed-by: thartmann, jbhateja, epeter --- src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp | 26 ++-- .../intrinsics/string/TestCountPositives.java | 125 +++++++++++------- .../intrinsics/string/TestHasNegatives.java | 97 ++++++++------ 3 files changed, 154 insertions(+), 94 deletions(-) diff --git a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp index 2154601f2f2..29413e5457c 100644 --- a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp +++ b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp @@ -3853,13 +3853,11 @@ void C2_MacroAssembler::count_positives(Register ary1, Register len, VM_Version::supports_bmi2()) { Label test_64_loop, test_tail, BREAK_LOOP; - Register tmp3_aliased = len; - movl(tmp1, len); vpxor(vec2, vec2, vec2, Assembler::AVX_512bit); - andl(tmp1, 64 - 1); // tail count (in chars) 0x3F - andl(len, ~(64 - 1)); // vector count (in chars) + andl(tmp1, 0x0000003f); // tail count (in chars) 0x3F + andl(len, 0xffffffc0); // vector count (in chars) jccb(Assembler::zero, test_tail); lea(ary1, Address(ary1, len, Address::times_1)); @@ -3879,12 +3877,17 @@ void C2_MacroAssembler::count_positives(Register ary1, Register len, testl(tmp1, -1); jcc(Assembler::zero, DONE); + + // check the tail for absense of negatives // ~(~0 << len) applied up to two times (for 32-bit scenario) #ifdef _LP64 - mov64(tmp3_aliased, 0xFFFFFFFFFFFFFFFF); - shlxq(tmp3_aliased, tmp3_aliased, tmp1); - notq(tmp3_aliased); - kmovql(mask2, tmp3_aliased); + { + Register tmp3_aliased = len; + mov64(tmp3_aliased, 0xFFFFFFFFFFFFFFFF); + shlxq(tmp3_aliased, tmp3_aliased, tmp1); + notq(tmp3_aliased); + kmovql(mask2, tmp3_aliased); + } #else Label k_init; jmp(k_init); @@ -3916,8 +3919,13 @@ void C2_MacroAssembler::count_positives(Register ary1, Register len, ktestq(mask1, mask2); jcc(Assembler::zero, DONE); + // do a full check for negative registers in the tail + movl(len, tmp1); // tmp1 holds low 6-bit from original len; + // ary1 already pointing to the right place + jmpb(TAIL_START); + bind(BREAK_LOOP); - // At least one byte in the last 64 bytes is negative. + // At least one byte in the last 64 byte block was negative. // Set up to look at the last 64 bytes as if they were a tail lea(ary1, Address(ary1, len, Address::times_1)); addptr(result, len); diff --git a/test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java b/test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java index afc308c37dd..76ef4766159 100644 --- a/test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java +++ b/test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2022, 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 @@ -21,88 +21,121 @@ * questions. */ -package compiler.intrinsics.string; - /* * @test - * @bug 8999999 + * @bug 8281146 * @summary Validates StringCoding.countPositives intrinsic with a small range of tests. + * @key randomness * @library /compiler/patches + * @library /test/lib * * @build java.base/java.lang.Helper * @run main compiler.intrinsics.string.TestCountPositives */ +/* + * @test + * @bug 8281146 8318509 + * @summary Validates StringCoding.countPositives intrinsic for AVX3 works with and without + * AVX3Threshold=0 + * @key randomness + * @library /compiler/patches + * @library /test/lib + * + * @build java.base/java.lang.Helper + * @requires vm.cpu.features ~= ".*avx512.*" + * @run main/othervm/timeout=1200 -XX:UseAVX=3 compiler.intrinsics.string.TestCountPositives + * @run main/othervm/timeout=1200 -XX:UseAVX=3 -XX:+UnlockDiagnosticVMOptions -XX:AVX3Threshold=0 compiler.intrinsics.string.TestCountPositives + */ +/** + * This test was derived from compiler.intrinsics.string.TestHasNegatives + */ +package compiler.intrinsics.string; + +import java.lang.Helper; +import java.util.Random; +import java.util.stream.IntStream; + +import jdk.test.lib.Utils; public class TestCountPositives { - private static byte[] tBa = new byte[4096 + 16]; + private static byte[] bytes = new byte[4096 + 32]; + + private static final Random RANDOM = Utils.getRandomInstance(); /** * Completely initialize the test array, preparing it for tests of the * StringCoding.hasNegatives method with a given array segment offset, - * length, and number of negative bytes. + * length, and number of negative bytes. The lowest index that will be + * negative is marked by negOffset */ - public static void initialize(int off, int len, int neg) { - assert (len + off <= tBa.length); + public static void initialize(int off, int len, int neg, int negOffset) { + assert (len + off <= bytes.length); // insert "canary" (negative) values before offset for (int i = 0; i < off; ++i) { - tBa[i] = (byte) (((i + 15) & 0x7F) | 0x80); + bytes[i] = (byte) (((i + 15) & 0x7F) | 0x80); } // fill the array segment for (int i = off; i < len + off; ++i) { - tBa[i] = (byte) (((i - off + 15) & 0x7F)); + bytes[i] = (byte) (((i - off + 15) & 0x7F)); } if (neg != 0) { // modify a number (neg) disparate array bytes inside // segment to be negative. - int div = (neg > 1) ? (len - 1) / (neg - 1) : 0; - int idx; - for (int i = 0; i < neg; ++i) { - idx = off + (len - 1) - div * i; - tBa[idx] = (byte) (0x80 | tBa[idx]); + for (int i = 0; i < neg; i++) { + int idx = off + RANDOM.nextInt(len - negOffset) + negOffset; + bytes[idx] = (byte) (0x80 | bytes[idx]); } } // insert "canary" negative values after array segment - for (int i = len + off; i < tBa.length; ++i) { - tBa[i] = (byte) (((i + 15) & 0x7F) | 0x80); + for (int i = len + off; i < bytes.length; ++i) { + bytes[i] = (byte) (((i + 15) & 0x7F) | 0x80); } } - /** Sizes of array segments to test. */ - private static int sizes[] = { 1, 2, 3, 4, 5, 6, 7, 8, 10, 11, 13, 17, 19, 23, 37, 61, 131, - 4099 }; - /** * Test different array segment sizes, offsets, and number of negative * bytes. */ public static void test_countPositives() throws Exception { - int len, off; - int ng; - - for (ng = 0; ng < 57; ++ng) { // number of negatives in array segment - for (off = 0; off < 8; ++off) { // starting offset of array segment - for (int i = 0; i < sizes.length; ++i) { // array segment size - // choice - len = sizes[i]; - if (len + off > tBa.length) - continue; - initialize(off, len, ng); - int calculated = Helper.StringCodingCountPositives(tBa, off, len); - int expected = countPositives(tBa, off, len); - if (calculated != expected) { - if (expected != len && calculated >= 0 && calculated < expected) { - // allow intrinsics to return early with a lower value, - // but only if we're not expecting the full length (no - // negative bytes) - continue; - } - throw new Exception("Failed test countPositives " + "offset: " + off + " " - + "length: " + len + " " + "return: " + calculated + " expected: " + expected + " negatives: " - + ng); - } - } + for (int off = 0; off < 16; off++) { // starting offset of array segment + // Test all array segment sizes 1-63 + for (int len = 1; len < 64; len++) { + test_countPositives(off, len, 0, 0); + test_countPositives(off, len, 1, 0); + test_countPositives(off, len, RANDOM.nextInt(30) + 2, 0); } + // Test a random selection of sizes between 64 and 4099, inclusive + for (int i = 0; i < 20; i++) { + int len = 64 + RANDOM.nextInt(4100 - 64); + test_countPositives(off, len, 0, 0); + test_countPositives(off, len, 1, 0); + test_countPositives(off, len, RANDOM.nextInt(len) + 2, 0); + } + for (int len : new int[] { 128, 2048 }) { + // test with negatives only in a 1-63 byte tail + int tail = RANDOM.nextInt(63) + 1; + int ng = RANDOM.nextInt(tail) + 1; + test_countPositives(off, len + tail, ng, len); + } + } + } + + private static void test_countPositives(int off, int len, int ng, int ngOffset) throws Exception { + assert (len + off < bytes.length); + initialize(off, len, ng, ngOffset); + int calculated = Helper.StringCodingCountPositives(bytes, off, len); + int expected = countPositives(bytes, off, len); + if (calculated != expected) { + if (expected != len && ng >= 0 && calculated >= 0 && calculated < expected) { + // allow intrinsics to return early with a lower value, + // but only if we're not expecting the full length (no + // negative bytes) + return; + } + throw new Exception("Failed test countPositives " + "offset: " + off + " " + + "length: " + len + " " + "return: " + calculated + " expected: " + expected + " negatives: " + + ng + " offset: " + ngOffset); } } diff --git a/test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java b/test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java index d73ea3f0139..6edf2dc2e56 100644 --- a/test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java +++ b/test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 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 @@ -26,20 +26,39 @@ * @bug 8054307 * @summary Validates StringCoding.hasNegatives intrinsic with a small range of tests. * @library /compiler/patches + * @library /test/lib * * @build java.base/java.lang.Helper * @run main compiler.intrinsics.string.TestHasNegatives */ +/* + * @test + * @bug 8054307 8318509 + * @summary Validates StringCoding.hasNegatives intrinsic for AVX3 works with and without + * AVX3Threshold=0 + * @key randomness + * @library /compiler/patches + * @library /test/lib + * + * @build java.base/java.lang.Helper + * @requires vm.cpu.features ~= ".*avx512.*" + * @run main/othervm/timeout=1200 -XX:UseAVX=3 compiler.intrinsics.string.TestHasNegatives + * @run main/othervm/timeout=1200 -XX:UseAVX=3 -XX:+UnlockDiagnosticVMOptions -XX:AVX3Threshold=0 compiler.intrinsics.string.TestHasNegatives + */ package compiler.intrinsics.string; -/* - * @summary Validates StringCoding.hasNegatives intrinsic with a small - * range of tests. - */ +import java.lang.Helper; +import java.util.Random; +import java.util.stream.IntStream; + +import jdk.test.lib.Utils; + public class TestHasNegatives { - private static byte[] tBa = new byte[4096 + 16]; + private static byte[] bytes = new byte[4096 + 32]; + + private static final Random RANDOM = Utils.getRandomInstance(); /** * Completely initialize the test array, preparing it for tests of the @@ -47,60 +66,60 @@ public class TestHasNegatives { * length, and number of negative bytes. */ public static void initialize(int off, int len, int neg) { - assert (len + off <= tBa.length); + assert (len + off <= bytes.length); // insert "canary" (negative) values before offset for (int i = 0; i < off; ++i) { - tBa[i] = (byte) (((i + 15) & 0x7F) | 0x80); + bytes[i] = (byte) (((i + 15) & 0x7F) | 0x80); } // fill the array segment for (int i = off; i < len + off; ++i) { - tBa[i] = (byte) (((i - off + 15) & 0x7F)); + bytes[i] = (byte) (((i - off + 15) & 0x7F)); } if (neg != 0) { // modify a number (neg) disparate array bytes inside // segment to be negative. - int div = (neg > 1) ? (len - 1) / (neg - 1) : 0; - int idx; - for (int i = 0; i < neg; ++i) { - idx = off + (len - 1) - div * i; - tBa[idx] = (byte) (0x80 | tBa[idx]); + for (int i = 0; i < neg; i++) { + int idx = off + RANDOM.nextInt(len); + bytes[idx] = (byte) (0x80 | bytes[idx]); } } // insert "canary" negative values after array segment - for (int i = len + off; i < tBa.length; ++i) { - tBa[i] = (byte) (((i + 15) & 0x7F) | 0x80); + for (int i = len + off; i < bytes.length; ++i) { + bytes[i] = (byte) (((i + 15) & 0x7F) | 0x80); } } - /** Sizes of array segments to test. */ - private static int sizes[] = { 1, 2, 3, 4, 5, 6, 7, 8, 10, 11, 13, 17, 19, 23, 37, 61, 131, - 4099 }; - /** * Test different array segment sizes, offsets, and number of negative * bytes. */ public static void test_hasNegatives() throws Exception { - int len, off; - int ng; - boolean r; - - for (ng = 0; ng < 57; ++ng) { // number of negatives in array segment - for (off = 0; off < 8; ++off) { // starting offset of array segment - for (int i = 0; i < sizes.length; ++i) { // array segment size - // choice - len = sizes[i]; - if (len + off > tBa.length) - continue; - initialize(off, len, ng); - r = Helper.StringCodingHasNegatives(tBa, off, len); - if (r ^ ((ng == 0) ? false : true)) { - throw new Exception("Failed test hasNegatives " + "offset: " + off + " " - + "length: " + len + " " + "return: " + r + " " + "negatives: " - + ng); - } - } + for (int off = 0; off < 16; off++) { // starting offset of array segment + // Test all array segment sizes 1-63 + for (int len = 1; len < 64; len++) { + test_hasNegatives(off, len, 0); + test_hasNegatives(off, len, 1); + test_hasNegatives(off, len, RANDOM.nextInt(30) + 2); } + // Test a random selection of sizes between 64 and 4099, inclusive + for (int i = 0; i < 20; i++) { + int len = 64 + RANDOM.nextInt(4100 - 64); + test_hasNegatives(off, len, 0); + test_hasNegatives(off, len, 1); + test_hasNegatives(off, len, RANDOM.nextInt(len) + 2); + } + } + } + + private static void test_hasNegatives(int off, int len, int maxNegatives) throws Exception { + assert (len + off < bytes.length); + initialize(off, len, maxNegatives); + boolean expected = (maxNegatives > 0); + boolean actual = Helper.StringCodingHasNegatives(bytes, off, len); + if (actual != expected) { + throw new Exception("Failed test hasNegatives " + "offset: " + off + " " + + "length: " + len + " " + "return: " + actual + " " + "negatives: " + + maxNegatives); } }