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

[Fei Yang]

On Fri, 7 Mar 2025 11:42:34 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 two additional commits since the last revision:
> 
>  - clean
>  - renaming

Thanks for the update. There minor comments remain. Seems fine to me otherwise.

src/hotspot/cpu/riscv/riscv.ad line 4975:

> 4973: 
> 4974:   ins_pipe(fp_load_constant_s);
> 4975: %}

Can we put this two after `loadConD0`? Then we have a more consistent order for the three variants (F->D->FH) at each place.

src/hotspot/cpu/riscv/riscv.ad line 8324:

> 8322: %}
> 8323: 
> 8324: instruct min_max_HF_reg(fRegF dst, fRegF src1, fRegF src2)

It will be easier for people to map to exitsting F/D variants if we use similar names like `sqrtHF_reg`, `minHF_reg_reg`, `maxHF_reg_reg`, `maddHF_reg_reg`, and `binOpsHF_reg_reg` for the the HF variant.

src/hotspot/cpu/riscv/vm_version_riscv.hpp line 301:

> 299:   static bool supports_fencei_barrier() { return ext_Zifencei.enabled(); }
> 300: 
> 301:   static bool supports_float16_float_conversion() {

Is it safe to simply rename this as `supports_float16` like other CPUs? I suppose it will still work in functionality even if we only have the `Zfhmin` extension, right?

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

PR Review: https://git.openjdk.org/jdk/pull/23844#pullrequestreview-2669705559
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1986554531
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1986550295
PR Review Comment: https://git.openjdk.org/jdk/pull/23844#discussion_r1986557022