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

[Fei Yang]

On Thu, 6 Mar 2025 14:30:40 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review this patch?
>> It's an implementation of https://github.com/openjdk/jdk/pull/22754 on riscv.
>> 
>> ## Performance
>> 
>> data
>> <google-sheets-html-origin style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); font-style: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;">
>> Benchmark | (vectorDim) | Mode | Cnt | Score -master | Error | Score - patch | Error | Units | Improvement (master/patch)
>> -- | -- | -- | -- | -- | -- | -- | -- | -- | --
>> Float16OperationsBenchmark.absBenchmark | 256 | avgt | 10 | 219.564 | 0.076 | 219.597 | 0.081 | ns/op | 1
>> Float16OperationsBenchmark.absBenchmark | 512 | avgt | 10 | 358.873 | 0.575 | 355.011 | 0.07 | ns/op | 1.011
>> Float16OperationsBenchmark.absBenchmark | 1024 | avgt | 10 | 582.361 | 0.189 | 581.832 | 0.006 | ns/op | 1.001
>> Float16OperationsBenchmark.absBenchmark | 2048 | avgt | 10 | 1035.633 | 0.239 | 1034.854 | 0.284 | ns/op | 1.001
>> Float16OperationsBenchmark.addBenchmark | 256 | avgt | 10 | 4951.702 | 0.194 | 2593.835 | 0.066 | ns/op | 1.909
>> Float16OperationsBenchmark.addBenchmark | 512 | avgt | 10 | 9867.909 | 0.314 | 5167.568 | 0.162 | ns/op | 1.91
>> Float16OperationsBenchmark.addBenchmark | 1024 | avgt | 10 | 21324.318 | 1.651 | 10016.456 | 1.07 | ns/op | 2.129
>> Float16OperationsBenchmark.addBenchmark | 2048 | avgt | 10 | 42618.969 | 3.877 | 19985.662 | 1.233 | ns/op | 2.132
>> Float16OperationsBenchmark.cosineSimilarityDequantizedFP16 | 256 | avgt | 10 | 2811.45 | 0.441 | 2701.419 | 140.699 | ns/op | 1.041
>> Float16OperationsBenchmark.cosineSimilarityDequantizedFP16 | 512 | avgt | 10 | 5568.561 | 0.654 | 5577.598 | 1.123 | ns/op | 0.998
>> Float16OperationsBenchmark.cosineSimilarityDequantizedFP16 | 1024 | avgt | 10 | 11109.108 | 1.7 | 11095.644 | 0.644 | ns/op | 1.001
>> Float16OperationsBenchmark.cosineSimilarityDequantizedFP16 | 2048 | avgt | 10 | 20017.095 | 0.778 | 21560.165 | 0.515 | ns/op | 0.928
>> Float16OperationsBenchmark.cosineSimilarityDoubleRoundingFP16 | 256 | avgt | 10 | 20864.303 | 23.768 | 1345.192 | 0.274 | ns/op | 15.51
>> Float16OperationsBenchmark.cosineSimilarityDoubleRoundingFP16 | 512 | avgt | 10 | 43596.262 | 102.075 | 2580.035 | 0.397 | ns/op | 16.898
>> Float16OperationsBenchmark.cosineSimilarityDoubleRoundingFP16 | 1024 | avgt | 10 | 91565.81...
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove TestFloat16VectorConvChain test for riscv temporariely

Several more comments after a closer look.

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.

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)`

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.

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;`

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

PR Review: https://git.openjdk.org/jdk/pull/23844#pullrequestreview-2665957697
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984266099
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984269266
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984275746
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1984256375