Skip to content

Commit

Permalink
8297689: Fix incorrect result of Short.reverseBytes() call in loops
Browse files Browse the repository at this point in the history
Reviewed-by: thartmann, jbhateja
  • Loading branch information
Pengfei Li committed Dec 6, 2022
1 parent f8f4630 commit a613998
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 6 deletions.
12 changes: 10 additions & 2 deletions src/hotspot/share/opto/vectornode.cpp
Expand Up @@ -169,10 +169,18 @@ int VectorNode::opcode(int sopc, BasicType bt) {
case Op_ReverseL:
return (is_integral_type(bt) ? Op_ReverseV : 0);
case Op_ReverseBytesS:
case Op_ReverseBytesUS:
// Subword operations in superword usually don't have precise info
// about signedness. But the behavior of reverseBytes for short and
// char are exactly the same.
return ((bt == T_SHORT || bt == T_CHAR) ? Op_ReverseBytesV : 0);
case Op_ReverseBytesI:
// There is no reverseBytes() in Byte class but T_BYTE may appear
// in VectorAPI calls. We still use ReverseBytesI for T_BYTE to
// ensure vector intrinsification succeeds.
return ((bt == T_INT || bt == T_BYTE) ? Op_ReverseBytesV : 0);
case Op_ReverseBytesL:
case Op_ReverseBytesUS:
return (is_integral_type(bt) ? Op_ReverseBytesV : 0);
return (bt == T_LONG ? Op_ReverseBytesV : 0);
case Op_CompressBits:
// Not implemented. Returning 0 temporarily
return 0;
Expand Down
9 changes: 7 additions & 2 deletions src/hotspot/share/prims/vectorSupport.cpp
Expand Up @@ -520,8 +520,13 @@ int VectorSupport::vop2ideal(jint id, BasicType bt) {
}
case VECTOR_OP_REVERSE_BYTES: {
switch (bt) {
case T_BYTE:
case T_SHORT:
case T_SHORT: return Op_ReverseBytesS;
// Superword requires type consistency between the ReverseBytes*
// node and the data. But there's no ReverseBytesB node because
// no reverseBytes() method in Java Byte class. T_BYTE can only
// appear in VectorAPI calls. We reuse Op_ReverseBytesI for this
// to ensure vector intrinsification succeeds.
case T_BYTE: // Intentionally fall-through
case T_INT: return Op_ReverseBytesI;
case T_LONG: return Op_ReverseBytesL;
default: fatal("REVERSE_BYTES: %s", type2name(bt));
Expand Down
Expand Up @@ -25,8 +25,7 @@
* @bug 8288112
* @summary Auto-vectorization of ReverseBytes operations.
* @requires vm.compiler2.enabled
* @requires vm.cpu.features ~= ".*avx2.*"
* @requires os.simpleArch == "x64"
* @requires (os.simpleArch == "x64" & vm.cpu.features ~= ".*avx2.*") | os.simpleArch == "AArch64"
* @library /test/lib /
* @run driver compiler.vectorization.TestReverseBytes
*/
Expand Down
102 changes: 102 additions & 0 deletions test/hotspot/jtreg/compiler/vectorization/TestSubwordReverseBytes.java
@@ -0,0 +1,102 @@
/*
* Copyright (c) 2022, Arm Limited. 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.
*/

package compiler.vectorization;

import compiler.lib.ir_framework.*;

/*
* @test
* @bug 8297689
* @summary Test miscompilation of reverseBytes call from subword types
* @library /test/lib /
* @requires vm.compiler2.enabled
* @run driver compiler.vectorization.TestSubwordReverseBytes
*/

public class TestSubwordReverseBytes {
private static final int SIZE = 32000;

private static int[] idx = new int[SIZE];
private static short[] rbs = new short[SIZE];
private static char[] rbc = new char[SIZE];

static {
for (int i = 0; i < SIZE; i++) {
idx[i] = i;
}
for (short s = 0; s < SIZE; s++) {
rbs[s] = Short.reverseBytes(s);
}
for (char c = 0; c < SIZE; c++) {
rbc[c] = Character.reverseBytes(c);
}
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_V})
public static int[] testShortReverseBytes() {
int[] res = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Short.reverseBytes((short) idx[i]);
}
return res;
}

@Run(test = "testShortReverseBytes")
public static void testShort() {
int[] res = testShortReverseBytes();
for (int i = 0; i < SIZE; i++) {
if (res[i] != rbs[i]) {
throw new RuntimeException("Wrong result: expected = " +
(int) rbs[i] + ", actual = " + res[i]);
}
}
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_V})
public static int[] testCharacterReverseBytes() {
int[] res = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Character.reverseBytes((char) idx[i]);
}
return res;
}

@Run(test = "testCharacterReverseBytes")
public static void testChar() {
int[] res = testCharacterReverseBytes();
for (int i = 0; i < SIZE; i++) {
if (res[i] != rbc[i]) {
throw new RuntimeException("Wrong result: expected = " +
(int) rbc[i] + ", actual = " + res[i]);
}
}
}

public static void main(String[] args) {
TestFramework.run();
}
}

Expand Up @@ -47,15 +47,18 @@ public class BasicCharOpTest extends VectorizationTestRunner {
private char[] a;
private char[] b;
private char[] c;
private int[] idx;

public BasicCharOpTest() {
a = new char[SIZE];
b = new char[SIZE];
c = new char[SIZE];
idx = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
a[i] = (char) (20 * i);
b[i] = (char) (i + 44444);
c[i] = (char) 10000;
idx[i] = i;
}
}

Expand Down Expand Up @@ -189,5 +192,24 @@ public char[] vectorUnsignedShiftRight() {
}
return res;
}

// ------------- ReverseBytes -------------
@Test
public char[] reverseBytesWithChar() {
char[] res = new char[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Character.reverseBytes(a[i]);
}
return res;
}

@Test
public int[] reverseBytesWithInt() {
int[] res = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Character.reverseBytes((char) idx[i]);
}
return res;
}
}

Expand Up @@ -47,15 +47,18 @@ public class BasicShortOpTest extends VectorizationTestRunner {
private short[] a;
private short[] b;
private short[] c;
private int[] idx;

public BasicShortOpTest() {
a = new short[SIZE];
b = new short[SIZE];
c = new short[SIZE];
idx = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
a[i] = (short) (-12 * i);
b[i] = (short) (9 * i + 8888);
c[i] = (short) -32323;
idx[i] = i;
}
}

Expand Down Expand Up @@ -187,5 +190,24 @@ public short[] vectorUnsignedShiftRight() {
}
return res;
}

// ------------- ReverseBytes -------------
@Test
public short[] reverseBytesWithShort() {
short[] res = new short[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Short.reverseBytes(a[i]);
}
return res;
}

@Test
public int[] reverseBytesWithInt() {
int[] res = new int[SIZE];
for (int i = 0; i < SIZE; i++) {
res[i] = Short.reverseBytes((short) idx[i]);
}
return res;
}
}

1 comment on commit a613998

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.