RFR: JDK-8318228: RISC-V: C2 ConvF2HF

[Hamlin Li]

On Thu, 18 Jan 2024 07:07:30 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Hi,
>> Can you review the patch to add ConvF2HF intrinsic to JDK for riscv？
>> Thanks!
>> 
>> ## Test
>> ### Functionality
>> #### hotspot tests
>> test/hotspot/jtreg/compiler/intrinsics/ 
>> test/hotspot/jtreg/compiler/c2/irTests
>> 
>> #### jdk tests
>> test/jdk/java/lang/Float/Binary16Conversion*.java
>> 
>> ### Performance
>> tested on licheepi.
>> 
>> #### with UseZfh enabled
>> 
>> Benchmark                                     (size)  Mode  Cnt     Score   Error  Units
>> Fp16ConversionBenchmark.floatToFloat16          2048  avgt    2  4170.549          ns/op
>> Fp16ConversionBenchmark.floatToFloat16Memory    2048  avgt    2    21.492          ns/op
>> 
>> 
>> #### with UseZfh disabled
>> (i.e. disable the intrinsic)
>> 
>> Benchmark                                     (size)  Mode  Cnt      Score   Error  Units
>> Fp16ConversionBenchmark.floatToFloat16          2048  avgt    2  25036.647          ns/op
>> Fp16ConversionBenchmark.floatToFloat16Memory    2048  avgt    2     27.326          ns/op
>
> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1842:
> 
>> 1840: 
>> 1841:   // preserve the payloads of non-canonical NaNs.
>> 1842:   __ srai(dst, dst, 13);
> 
> I see the lowest 13 bits of the payload for `src` is simply discarded here. But these bits are also used for calculating the new significand bits for float16 [1]. So this doesn't seem OK to me. Did I miss anything?
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Float.java#L1112-L1113

It's discarded intentionally, just like in HF2F it's [padded with zero in lower 13 bits](https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp#L1800)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17450#discussion_r1459107338