RFR: 8345298: RISC-V: Add riscv backend for Float16 operations - scalar [v4]

[Hamlin Li]

On Fri, 7 Mar 2025 01:23:24 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remove TestFloat16VectorConvChain test for riscv temporariely
>
> src/hotspot/cpu/riscv/assembler_riscv.hpp line 339:
> 
>> 337:     single_precision,
>> 338:     double_precision
>> 339:   };
> 
> Seems better to move this enum to file `macroAssembler_riscv.hpp`? It's only used in the macro assember routines.

OK.

> src/hotspot/cpu/riscv/riscv.ad line 8254:
> 
>> 8252: // half precision operations
>> 8253: 
>> 8254: instruct reinterpretS2HF(fRegF dst, iRegINoSp src)
> 
> Suggestion: `instruct reinterpretS2HF(fRegF dst, iRegI src)`

OK.

> src/hotspot/cpu/riscv/riscv.ad line 8257:
> 
>> 8255: %{
>> 8256:   match(Set dst (ReinterpretS2HF src));
>> 8257:   effect(TEMP_DEF dst);
> 
> I don't see why this `TEMP_DEF dst` effect is needed. Maybe we should remove it from all the newly-added instructs.

OK.

> src/hotspot/cpu/riscv/vm_version_riscv.cpp line 474:
> 
>> 472:   case vmIntrinsics::_floatToFloat16:
>> 473:   case vmIntrinsics::_float16ToFloat:
>> 474:     if (!supports_float16_to_float()) {
> 
> As the `supports_float16_to_float` function name doesn't reflect the `_floatToFloat16` case, I suggest to directly inline the code here: `return UseZfh || UseZfhmin;`

Seems to me it's better to keep it in a method, as it's used in several places. I'll change it to supports_float16_float_conversion for naming improvement.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984917624
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984917699
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984917833
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984917519