RFR: 8318227: RISC-V: C2 ConvHF2F [v2]

[Fei Yang]

On Tue, 28 Nov 2023 15:13:25 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1704:
>> 
>>> 1702:   // check whether it's a NaN.
>>> 1703:   mv(t0, 0x7c00);
>>> 1704:   andr(tmp, src, t0);
>> 
>> I see from the exponent encoding of float16 on [1], it could be a negative/positive infinity as well when exponent is 0b11111. It depends on whether the significand is zero or not. So it this checking for NAN sufficient?
>> 
>> [1] https://en.wikipedia.org/wiki/Half-precision_floating-point_format
>
> Goot catch!
> 
> Your observation is right and wrong. :)
> We could have a patch like below, but it will scrafise the performance of the normal case(non-NaN, non-Inf), as it adds one extra instructions at the critical path.
> Maybe a solution is to add some comments here, to state that NaN and Inf are processed in slow path, and slow path is necessary to NaN, but not necessary to Inf.
> How do you think about it?
> 
> 
> $ git diff src/
> diff --git a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> index 1b6140242b8..7413767395f 100644
> --- a/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> +++ b/src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp
> @@ -1700,10 +1700,11 @@ void C2_MacroAssembler::float16_to_float(FloatRegister dst, Register src, Regist
>    auto stub = C2CodeStub::make<FloatRegister, Register, Register>(dst, src, tmp, 20, float16_to_float_nan_path);
>  
>    // check whether it's a NaN.
> -  mv(t0, 0x7c00);
> +  mv(t0, 0x7fff);
>    andr(tmp, src, t0);
> +  mv(t0, 0x7c00);
>    // jump to stub processing NaN case
> -  beq(t0, tmp, stub->entry());
> +  bgt(tmp, t0, stub->entry());
>  
>    // non-NaN cases, just use built-in instructions.
>    fmv_h_x(dst, src);

Ah, I haven't checked the slow-path yet. Could it handle negative/positive infinity cases too? If that is true, then we might give it a new name instead of `float16_to_float_nan_path` which might indicates a path only for NAN-handling.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16802#discussion_r1409000423