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

[Fei Yang]

On Tue, 16 Jan 2024 18:43:03 GMT, Hamlin Li <mli 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

Hi, Thanks for this change. I have a small question.

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

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

PR Review: https://git.openjdk.org/jdk/pull/17450#pullrequestreview-1828999418
PR Review Comment: https://git.openjdk.org/jdk/pull/17450#discussion_r1457016454