RFR: 8365206: RISC-V: compiler/c2/irTests/TestFloat16ScalarOperations.java is failing on riscv64

[Fei Yang]

On Tue, 19 Aug 2025 08:22:59 GMT, Hamlin Li <mli at openjdk.org> wrote:

> Hi,
> Can you help to review this patch?
> 
> Currently implementation of conversion from float to float16 only preserve some significant bits of a NaN, which is not right in some cases.
> As this (NaN conversion from float to float16) is already in the slow path, so I'll just fix it by preserving all significant bits in the same way as j.l.Float.floatToFloat16.
> As current tests does not catch the issue, except of TestFloat16ScalarOperations.java, so add another test.
> 
> There is also the similar issue in vector version of conversion from float to float16, I'll address it in another pr [JDK-8365772](https://bugs.openjdk.org/browse/JDK-8365772)
> 
> Thanks!

Thanks for fixing this! I have several comments.
BTW: I assume you have checked `float16_to_float_slow_path` as well? Does it need a fix?

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 2394:

> 2392:   __ bind(stub.entry());
> 2393: 
> 2394:   __ float_to_float16_NaN(src, dst, t0, t1);

I see `t1` which is the rFlagsReg register for riscv is passed as a temp register. But this side effect (clobbering of `t1`) doesn't reflect on the callsite `float_to_float16` in `instruct convF2HF_reg_reg`. So I guess you might want to pass `tmp` instead of `t1` here. Like: `__ float_to_float16_NaN(src, dst, t0, tmp);`

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 5958:

> 5956: 
> 5957: // this is a utility method to process the slow path of NaN when converting float to float16
> 5958: // check j.l.Float.floatToFloat16

Suggestion: `Helper routine processing the slow path of NaN when converting float to float16`

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 5959:

> 5957: // this is a utility method to process the slow path of NaN when converting float to float16
> 5958: // check j.l.Float.floatToFloat16
> 5959: void MacroAssembler::float_to_float16_NaN(FloatRegister src, Register dst,

Can you swap the first two params? I perfer to have `dst` which holds the result as the first one. I think that's what we always do for other assembler routines as well.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 5968:

> 5966: 
> 5967:   // preserve the payloads of non-canonical NaNs.
> 5968:   // get the result by merging sign bit and payloads of preserved non-canonical NaNs.

Maybe we can borrow some code comments from the java code?

// 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.

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 1434:

> 1432:   void java_round_double(Register dst, FloatRegister src, FloatRegister ftmp);
> 1433: 
> 1434:   // this is a utility method to process the slow path of NaN when converting float to float16

Suggestion: `Helper routine processing the slow path of NaN when converting float to float16`

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

PR Review: https://git.openjdk.org/jdk/pull/26838#pullrequestreview-3134487961
PR Review Comment: https://git.openjdk.org/jdk/pull/26838#discussion_r2286872903
PR Review Comment: https://git.openjdk.org/jdk/pull/26838#discussion_r2287256536
PR Review Comment: https://git.openjdk.org/jdk/pull/26838#discussion_r2286883081
PR Review Comment: https://git.openjdk.org/jdk/pull/26838#discussion_r2287251315
PR Review Comment: https://git.openjdk.org/jdk/pull/26838#discussion_r2286756857