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

[Bhavana Kilambi]

On Thu, 14 Nov 2024 09:43:49 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> 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...
>
> We can also update existing test for new conversion
> 
> test/hotspot/jtreg/compiler/c2/irTests/TestPhiDuplicatedConversion.java

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

Hi @jatin-bhateja , I think that check should be there. The lines 1905-1907 will be reachable when get_convert_type() is called again with the dest_type (`short`) when it is supposed to return `short`. If this check is not there, it will return `T_INT`. For the following example - 
`rng.nextBoolean() ? Float.floatToFloat16(a) : Float.floatToFloat16(b)
`
it will generate `ConvF2I` instead of `ConvF2HF`.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1283#discussion_r1843605982