RFR: 8342103: C2 compiler support for Float16 type and associated operations [v2]

[Jatin Bhateja]

On Mon, 25 Nov 2024 08:56:31 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> I heard no argument about why you did not split this up. Please do that in the future. It is hard to review well when there is this much code. If it is really necessary, then sure. Here it does not seem necessary to deliver all at once.
> 
> > The patch includes IR framework-based scalar constant folding test points.
> > You mention this IR test:
> > https://github.com/openjdk/jdk/pull/21490/files#diff-3f8786f9f62662eda4b4a5c76c01fa04534c94d870d496501bfc20434ad45579R169-R174
> 
> Here I only see the use of very trivial values. I think we need more complicated cases.
> 
> What about these:
> 
> * Add/Sub/Mul/Div/Min/Max ... with NaN and infinity.
> * Same where it would overflow the FP16 range.
> * Negative zero tests.
> * Division by powers of 2.
> 
> It would for example be nice if you could iterate over all inputs. FP16 with 2 inputs is only 32bits, that can be iterated in just a few seconds. Then you can run the computation with constants in the interpreter, and compare to the results in compiled code.

[ScalarFloat16OperationsTest.java](https://github.com/openjdk/jdk/pull/21490/files#diff-6afb7e66ce0fcdac61df60af0231010b20cf16489ec7e4d5b0b41852db8796a0) 
Adds has a specialized data provider that generates test vectors with special values, our functional validation is covering the entire Float16 value range.

> src/hotspot/share/opto/divnode.cpp line 789:
> 
>> 787: 
>> 788:   if(t1 == TypeH::ZERO && !g_isnan(t2->getf()) && t2->getf() != 0.0)
>> 789:     return TypeH::ZERO;
> 
> brackets for if
> 
> Ok, why not also do it for negative zero then?

Same as above,  IEEE 754 spec treats both  +ve and -ve zeros equally during comparison operations.

jshell> 0.0f != 0.0f
$1 ==> false

jshell> 0.0f != -0.0f
$2 ==> false

jshell> -0.0f != -0.0f
$3 ==> false

jshell> -0.0f != 0.0f
$4 ==> false

> src/hotspot/share/opto/divnode.cpp line 797:
> 
>> 795: //------------------------------isA_Copy---------------------------------------
>> 796: // Dividing by self is 1.
>> 797: // If the divisor is 1, we are an identity on the dividend.
> 
> Suggestion:
> 
> // If the divisor is 1, we are an identity on the dividend.
> 
> `Dividing by self is 1.` That does not seem to apply here. Maybe you meant `dividing by 1 is self`?

The comment mentions the divisor being 1. Looks fine.

> src/hotspot/share/opto/divnode.cpp line 836:
> 
>> 834: 
>> 835:   // return multiplication by the reciprocal
>> 836:   return (new MulHFNode(in(1), phase->makecon(TypeH::make(reciprocal))));
> 
> Do we have good tests for this optimization?

I have added a test point
https://github.com/openjdk/jdk/pull/21490/files#diff-3f8786f9f62662eda4b4a5c76c01fa04534c94d870d496501bfc20434ad45579R203

I also added detailed comments to explain this better.

> src/hotspot/share/opto/mulnode.cpp line 561:
> 
>> 559: const Type *MulHFNode::mul_ring(const Type *t0, const Type *t1) const {
>> 560:   if( t0 == Type::HALF_FLOAT || t1 == Type::HALF_FLOAT ) return Type::HALF_FLOAT;
>> 561:   return TypeH::make( t0->getf() * t1->getf() );
> 
> I hope that `TypeH::make` handles the overflow cases well... does it?
> And do we have tests for this?

Please refer to following lines of code.
https://github.com/openjdk/jdk/pull/21490/files#diff-3559dcf23b719805be5fd06fd5c1851dbd8f53e47afe6d99cba13a3de0ebc6b2R1446

There are two versions of TypeH::make, one with short and the other accepting floating point parameter, in the latter version we explicitly invoke a runtime help to convert float to float16 value, this shall take care of overflow scenario where we return an infinite Float16 value.  There is no underflow in the case of a floating point number, for graceful degradation we enter into a sub-normal range and eventually return a zero value. On the other end of the spectrum i.e -ve values range we return a NEGATIVE_INFINITE, existing runtime helper is fully equipped to handle these cases.

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

PR Comment: https://git.openjdk.org/jdk/pull/21490#issuecomment-2498908764
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857267174
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266958
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266304
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1857266117