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

[Hamlin Li]

On Fri, 19 Jan 2024 14:31:37 GMT, Hamlin Li <mli at openjdk.org> wrote:

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

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, [DK-8324212](https://bugs.openjdk.org/browse/JDK-8324212)

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

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