RFR: 8320646: RISC-V: C2 VectorCastHF2F [v6]

[Fei Yang]

On Tue, 5 Mar 2024 16:34:13 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you review this patch to add instrinsics for VectorCastHF2F/VectorCastF2HF?
>> Thanks!
>> 
>> ## Test
>> 
>> test/jdk/java/lang/Float/Binary16ConversionNaN.java
>> test/jdk/java/lang/Float/Binary16Conversion.java
>> 
>> hotspot/jtreg/compiler/intrinsics/float16
>> hotspot/jtreg/compiler/vectorization/TestFloatConversionsVectorjava
>> hotspot/jtreg/compiler/vectorization/TestFloatConversionsVectorNaN.java
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   filter tests with zvfh

Thanks for the quick update. Several minor comments remain after another look. Looks good otherwise.

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

> 1921:   vcpop_m(t0, v0);
> 1922: 
> 1923:   // non-NaN or non-Inf cases, just use built-in instructions.

Suggestion: s/non-NaN or non-Inf cases/For non-NaN or non-Inf cases/

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

> 1925: 
> 1926:   // jump to stub processing NaN and Inf cases if there is any of them in the vector-wide.
> 1927:   bgtz(t0, stub->entry());

I prefer to use `bnez` instead of `bgtz` here to be consistent with NaN check in `C2_MacroAssembler::float_to_float16_v`

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

> 1959: // j.l.Float.float16ToFloat
> 1960: void C2_MacroAssembler::float_to_float16_v(VectorRegister dst, VectorRegister src, VectorRegister tmp, uint length) {
> 1961:   assert_different_registers(dst, src, tmp);

Maybe rename `tmp` to `vtmp` to indicate that this is temp vector register?

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

> 1962: 
> 1963:   auto stub = C2CodeStub::make<VectorRegister, VectorRegister, VectorRegister, uint>
> 1964:               (dst, src, tmp, length, 36, float_to_float16_v_slow_path);

You might want to reduce the stub size from 36 to 28 after the stub changes you made.

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

> 1975:   // and also moving vsetvli_helper(..., mf2) here does not impact vcpop_m.
> 1976:   vsetvli_helper(BasicType::T_SHORT, length, Assembler::mf2);
> 1977:   vcpop_m(t0, v0);

It looks a bit weird to place this `vcpop_m` here even though it works in functionality. Can we reserve and pass another temp integer register to store the result of this `vcpop_m` and move this it immediately after `vmfne_vv`? Then the code will be more readable to me.

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

> 1977:   vcpop_m(t0, v0);
> 1978: 
> 1979:   // non-NaN cases, just use built-in instructions.

Suggestion: s/non-NaN cases/For non-NaN cases/

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

> 189: 
> 190:   void float16_to_float_v(VectorRegister dst, VectorRegister src, uint length);
> 191:   void float_to_float16_v(VectorRegister dst, VectorRegister src, VectorRegister tmp, uint length);

Can you rename `length` to `vector_length` to be consistent with other assember friends in naming?

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

Changes requested by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17698#pullrequestreview-1918718588
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1514141674
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1513984137
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1514051181
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1513873863
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1514135659
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1514140774
PR Review Comment: https://git.openjdk.org/jdk/pull/17698#discussion_r1513799254