RFR: 8322174: RISC-V: C2 VectorizedHashCode RVV Version [v24]

[Fei Yang]

On Mon, 11 Aug 2025 10:24:29 GMT, Yuri Gaevsky <duke at openjdk.org> wrote:

>> The patch adds possibility to use RVV instructions for faster vectorizedHashCode calculations on RVV v1.0.0 capable hardware.
>> 
>> Testing: hotspot/jtreg/compiler/ under QEMU-8.1 with RVV v1.0.0.
>
> Yuri Gaevsky has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed reservations for unused vector registers per reviewer's comment; added sanity assertion.

Thanks for the update. Having another look. Some comments along the way.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2006:

> 2004:   const int num_8bit_elems_in_vec_reg = MaxVectorSize;
> 2005:   // Let's use T_INT as all hashCode calculations eventually deal with ints.
> 2006:   const int ints_in_vec_reg = num_8bit_elems_in_vec_reg/sizeof(jint);

Suggestion: `const int ints_in_vec_reg = MaxVectorSize / sizeof(jint);`
We can remove `num_8bit_elems_in_vec_reg` then.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2014:

> 2012: 
> 2013:   switch (eltype) {
> 2014:   case T_BOOLEAN: BLOCK_COMMENT("arrays_hashcode_v(unsigned byte) {"); break;

Please leave two spaces for each case.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2028:

> 2026: 
> 2027:   const VectorRegister v_sum    = v2;
> 2028: 

Seems no need to have this new line here.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2032:

> 2030:   const VectorRegister v_coeffs = v6;
> 2031:   const VectorRegister v_tmp    = v8;
> 2032:   const VectorRegister v_zred = v_tmp;

Seems we don't really need this alias `v_zred`. We can use `v_tmp` instead.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2042:

> 2040: 
> 2041:   andi(t1, cnt, MAX_VEC_MASK);
> 2042:   beqz(t1, SCALAR_TAIL);

Suggestion: Use `t0` instead of `t1` to hold some short lived values.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2044:

> 2042:   beqz(t1, SCALAR_TAIL);
> 2043: 
> 2044:   vsetvli(t0, x0, Assembler::e32, Assembler::m2);

It will be safer to use `t1` instead of `t0` here to hold the number of elements processed for each round.
`t0` as a scratch register gets clobbered by various assembler routines more frequently than `t1`.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2061:

> 2059:   andi(t1, cnt, MAX_VEC_MASK);
> 2060:   mulw(result, result, pow31_highest);
> 2061:   bne(t1, x0, VEC_LOOP);

Suggestion: `bnez(t1, VEC_LOOP);`

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2110:

> 2108: }
> 2109: 
> 2110: void C2_MacroAssembler::arrays_hashcode_vec_elload(VectorRegister varr,

Can you rename this as `C2_MacroAssembler::arrays_hashcode_elload_v`? I think that will be more consistent in naming with other places. And I personally perfer `vdst` to `varr` the first param.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp line 102:

> 100:   void arrays_hashcode_v(Register ary, Register cnt, Register result,
> 101:                          Register tmp1, Register tmp2, Register tmp3,
> 102:                          BasicType eltype);

Please put a new line here.

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.hpp line 106:

> 104:   int arrays_hashcode_elsize(BasicType eltype);
> 105:   void arrays_hashcode_elload(Register dst, Address src, BasicType eltype);
> 106:   void arrays_hashcode_vec_elload(VectorRegister varr, VectorRegister vtmp, Register array, BasicType eltype);

Similar here. Consider renaming to `arrays_hashcode_elload_v`.

src/hotspot/cpu/riscv/riscv_v.ad line 4092:

> 4090:   match(Set result (VectorizedHashCode (Binary ary cnt) (Binary result basic_type)));
> 4091:   effect(USE_KILL ary, USE_KILL cnt, USE basic_type,
> 4092:          TEMP  v2, TEMP  v3, TEMP  v4, TEMP  v5, TEMP  v6, TEMP  v7, TEMP v8, TEMP v9,

Please keep only one space at RHS of each `TEMP`.

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 6587:

> 6585:     assert(UseRVV, "sanity");
> 6586:     const int num_8bit_elems_in_vec_reg = MaxVectorSize;
> 6587:     const int ints_in_vec_reg = num_8bit_elems_in_vec_reg/sizeof(jint);

Suggestion: `const int ints_in_vec_reg = MaxVectorSize / sizeof(jint);`
We can remove num_8bit_elems_in_vec_reg then.

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

PR Review: https://git.openjdk.org/jdk/pull/17413#pullrequestreview-3113812897
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272292124
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272218374
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272345113
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272234458
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272274301
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272270365
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272287930
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2271970183
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272227887
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272340129
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272222013
PR Review Comment: https://git.openjdk.org/jdk/pull/17413#discussion_r2272293705