RFR: JDK-8318228: RISC-V: C2 ConvF2HF [v2]

[Hamlin Li]

On Fri, 19 Jan 2024 15:15:10 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> 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)
>
> Right now, I'm not sure.
> I have below patch:
> 
> diff --git a/src/java.base/share/classes/java/lang/Float.java b/src/java.base/share/classes/java/lang/Float.java
> index 7508c22d7f4..f96e23b568e 100644
> --- a/src/java.base/share/classes/java/lang/Float.java
> +++ b/src/java.base/share/classes/java/lang/Float.java
> @@ -1108,9 +1108,7 @@ public static short floatToFloat16(float f) {
>                      // Preserve high order bit of float NaN in the
>                      // binary16 result NaN (tenth bit); OR in remaining
>                      // bits into lower 9 bits of binary 16 significand.
> -                    | (doppel & 0x007f_e000) >> 13 // 10 bits
> -                    | (doppel & 0x0000_1ff0) >> 4  //  9 bits
> -                    | (doppel & 0x0000_000f));     //  4 bits
> +                    | (doppel & 0x007f_e000) >> 13); // 10 bits
>          }
>  
>          float abs_f = Math.abs(f);
> 
> And, test/jdk/java/lang/Float/Binary16ConversionNaN.java/Binary16Conversion.java both passed.
> 
> Either the tests(both library and hotspot) + intrinsics (not sure if intrinsics on other platforms need improvement) needs to be improved, or the code in library needs to be simplified. (To be frank, I don't think NaN needs such a complicated spec/design, but it depends on the spec).
> 
> I just filed a library bug to discuss it, [JDK-8324212](https://bugs.openjdk.org/browse/JDK-8324212)

Per discussion at: https://mail.openjdk.org/pipermail/riscv-port-dev/2022-December/000706.html, when NaN is used as input and output is also NaN, then there is no restriction on the exact NaN number, this is confirmed by the java library team. That means we can return any NaN if the input is an NaN.

So, I think we're fine to just discard the last 13 bits (as implemented in this patch), and in this way it's convenient for us as this will help to pass all the existing tests in hotspot.

But, for long term and performance consideration, I think we need to re-visit all the NaN related intrinsics in riscv to check if we need to treat NaN specially rather than just leveraging the riscv default behaviour. And I filed a bug to track this task: [JDK-8324303](https://bugs.openjdk.org/browse/JDK-8324303)

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

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