[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR: 8297689: Fix incorrect result of Short.reverseBytes() call in loops
From:       Tobias Hartmann <thartmann () openjdk ! org>
Date:       2022-11-30 8:43:29
Message-ID: 8pV4gvVPCq8hnGreDD3Ex80UjgkTqd1PgePPH6zAUqQ=.309fbc0b-3486-41d9-9681-220c9eb51f7e () github ! com
[Download RAW message or body]

On Wed, 30 Nov 2022 07:20:11 GMT, Pengfei Li <pli@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


[prev in list] [next in list] [prev in thread] [next in thread]