RFR: 8297689: Fix incorrect result of Short.reverseBytes() call in loops

[Tobias Hartmann]

On Wed, 30 Nov 2022 07:20:11 GMT, Pengfei Li <pli at openjdk.org> wrote:

> Recently, we find calling `Short.reverseBytes()` in loops may generate incorrect result if the code is compiled by C2. Below is a simple case to reproduce.
> 
> 
> class Foo {
>   static final int SIZE = 50;
>   static int a[] = new int[SIZE];
> 
>   static void test() {
>     for (int i = 0; i < SIZE; i++) {
>       a[i] = Short.reverseBytes((short) a[i]);
>     }
>   }
> 
>   public static void main(String[] args) throws Exception {
>     Class.forName("java.lang.Short");
>     a[25] = 16;
>     test();
>     System.out.println(a[25]);
>   }
> }
> 
> // $ java -Xint Foo
> // 4096
> // $ java -Xcomp -XX:-TieredCompilation -XX:CompileOnly=Foo.test Foo
> // 268435456
> 
> 
> In this case, the `reverseBytes()` call is intrinsified and transformed into a `ReverseBytesS` node. But then C2 compiler incorrectly vectorizes it into `ReverseBytesV` with int type. C2 `Op_ReverseBytes*` has short, char, int and long versions. Their behaviors are different for different data sizes. In superword, subword operation itself doesn't have precise data size info. Instead, the data size info comes from memory operations in its use-def chain. Hence, vectorization of `reverseBytes()` is valid only if the data size is consistent with the type size of the caller's class. But current C2 compiler code lacks fine-grained type checks for `ReverseBytes*` in vector transformation. It results in `reverseBytes()` call from Short or Character class with int load/store gets vectorized incorrectly in above case.
> 
> To fix the issue, this patch adds more checks in `VectorNode::opcode()`. T_BYTE is a special case for `Op_ReverseBytes*`. As the Java Byte class doesn't have `reverseBytes()` method so there's no `Op_ReverseBytesB`. But T_BYTE may still appear in VectorAPI calls. In this patch we still use `Op_ReverseBytesI` for T_BYTE to ensure vector intrinsification succeeds.
> 
> Tested with hotspot::hotspot_all_no_apps, jdk tier1~3 and langtools tier1 on x86 and AArch64, no issue is found.

This looks reasonable to me but I'm not an expert in that code.

-------------

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11427