[lworld+fp16] RFR: 8341414: Add support for FP16 conversion routines [v2]

[Jatin Bhateja]

On Thu, 31 Oct 2024 13:50:40 GMT, Bhavana Kilambi <bkilambi at openjdk.org> wrote:

>> This patch adds intrinsic support for FP16 conversion routines to int/long/double and also the aarch64 backend support. This patch implements both scalar and vector versions for these conversions.
>> 
>> Performance numbers on aarch64 machine with SVE support :
>> 
>> 
>> Benchmark                         (vectorDim)   Gain
>> Float16OpsBenchmark.fp16ToDouble  1024          18.23
>> Float16OpsBenchmark.fp16ToInt     1024          1.93
>> Float16OpsBenchmark.fp16ToLong    1024          3.95
>> 
>> 
>> The Gain column is the ratio between thrpt of this patch and the thrpt with the intrinsics disabled (which generates FP32 arithmetic).
>
> Bhavana Kilambi has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove intrinsification of conversion methods in Float16

Hi @Bhavana-Kilambi ,
I have added few comments, patch looks good otherwise.

Best Regards,
Jatin

src/hotspot/share/opto/cfgnode.cpp line 1899:

> 1897: 
> 1898: // Returns the BasicType of a given convert node and a type, with special handling to ensure that conversions to
> 1899: // and from half float will return the SHORT basic type, as that wouldn't be returned typically from TypeInt.

Comment needs re-formatting, "Returns the BasicType of a given convert node based on its opcode or type", not related to this patch though.

src/hotspot/share/opto/cfgnode.cpp line 1907:

> 1905:     } else if (convert_op == Op_ConvHF2F || convert_op == Op_ConvF2HF || convert_op == Op_ConvHF2D || convert_op == Op_ConvHF2L) {
> 1906:       return T_SHORT;
> 1907:     }

This transformation is moving conversion across the Phi node thereby saving expensive conversion instructions.

`Phi (ConvHF2F src1) (ConvHF2F src2) =>   ConvHF2F  (Phi src1  src2)
`

For the Op_ConvF2HF case, type parameter will be of float type and thus we will never enter control flow between L1905 - L1907. 

I think it will be harmless if we remove convert_op == Op_ConvF2HF check...

src/hotspot/share/runtime/stubRoutines.hpp line 511:

> 509:     return ((hf2l_stub_t)_hf2l)(x);
> 510:   }
> 511: 

Instead of defining new value conversion stubs for hf2l/i/d, we can typecast the result of hf2f_stub to the appropriate target type and then bank on c-compiler value conversion.

test/jdk/java/lang/Float16/FP16ScalarOperations.java line 177:

> 175:             System.out.println("Incorrest result for FP16 to double conversion. Expected value:" + expected + " Actual value: " + actual);
> 176:         }
> 177:     }

Adding some constant folding cases covering the following Float16 values to test newly introduced value transforms will be helpful:-
-   Float16.NaN
-   Float16.MAX_VALUE (0x1.ffcP+15)
-   Float16.MIN_VALUE (0x1.0P-24)
-   Float16.MIN_NORMAL_VALUE (0x.1P-14)
-   Float16.POSITIVE_INFINITE 
-   Float16.NEGATIVE_INFINITE
-   And few cases in the normal value range.

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

PR Review: https://git.openjdk.org/valhalla/pull/1283#pullrequestreview-2435208304
PR Review Comment: https://git.openjdk.org/valhalla/pull/1283#discussion_r1841796183
PR Review Comment: https://git.openjdk.org/valhalla/pull/1283#discussion_r1841876074
PR Review Comment: https://git.openjdk.org/valhalla/pull/1283#discussion_r1841669929
PR Review Comment: https://git.openjdk.org/valhalla/pull/1283#discussion_r1841687136