RFR: 8318220: RISC-V: C2 ReverseI

[Hamlin Li]

On Wed, 12 Mar 2025 03:02:04 GMT, Fei Yang <fyang at openjdk.org> wrote:

> I was also trying to search this brev8. I think this will also make it possible to add related vector ones like "ReverseV" and "ReverseBytesV".

Yes, a later PR will implement it.

> src/hotspot/cpu/riscv/riscv_b.ad line 257:
> 
>> 255:     __ rev8($dst$$Register, $src$$Register);
>> 256:     __ brev8($dst$$Register, $dst$$Register);
>> 257:     __ srli($dst$$Register, $dst$$Register, 32);
> 
> Shouldn't this be an arithmetic shift-right operation (`srai`)? For example, we should have a negative value after reversing int value 1. Maybe the warmup iters for your newly-added test is too small to cover this?
> 
> 
> Test.java
> import java.lang.*;
> 
> public class Test {
> 
>     public static void main(String[] args) {
>         int a = 1;
>         System.out.println("Number = " + a);
> 
>         // It returns the value obtained by reversing order of the bits
>         System.out.println("By reversing we get = " + Integer.reverse(a));
>     }
> }
> 
> $java Test
> Number = 1
> By reversing we get = -2147483648

Interesting question.
`srli` will also result 0x80000000 which is a int of -2147483648, I think the question could be translated to something like, "do we need to sign-extend the result?" I think the answer should be YES, so you're right.
I'm thinking how to construct a test so that the current implementation fails. `long l = Integer.reverse(i)` could be a solution.

> src/hotspot/os_cpu/linux_riscv/riscv_hwprobe.cpp line 192:
> 
>> 190:   if (is_set(RISCV_HWPROBE_KEY_IMA_EXT_0, RISCV_HWPROBE_EXT_ZBKB)) {
>> 191:     VM_Version::ext_Zbkb.enable_feature();
>> 192:   }
> 
> Are we auto-enabling an experimental feature?

Thanks, I'll fix this.

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

PR Comment: https://git.openjdk.org/jdk/pull/23963#issuecomment-2717389625
PR Review Comment: https://git.openjdk.org/jdk/pull/23963#discussion_r1991156612
PR Review Comment: https://git.openjdk.org/jdk/pull/23963#discussion_r1991156510