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

[Emanuel Peter]

On Tue, 17 Dec 2024 11:05:18 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> src/hotspot/share/opto/divnode.cpp line 840:
>> 
>>> 838:   if (g_isnan(t1->getf()) || g_isnan(t2->getf())) {
>>> 839:     return TypeH::make(NAN);
>>> 840:   }
>> 
>> I'm a little confused here. We are working with nodes that have type Float16, but we are asking for Float constants here. Why is that, how does it work?
>
> Please refer to PhaseIGVN::transform, we create constant IR for singleton types. 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/phaseX.cpp#L721

That misses my question, I was again confused about float vs Float16. But I see from your previous answer that `getf` does the conversion from `Float16` -> `float`. So all good.

>> src/hotspot/share/opto/subnode.cpp line 566:
>> 
>>> 564:     return t1;
>>> 565:   }
>>> 566:   else if(g_isnan(t2->getf())) {
>> 
>> General question: why are you using `getf` and not `geth` all over the code?
>
> getf is a routine that performs half float to float conversion for TypeH.

Ah right, I see now. Thanks.

>> src/hotspot/share/opto/type.cpp line 1530:
>> 
>>> 1528: uint TypeH::hash(void) const {
>>> 1529:   return *(uint*)(&_f);
>>> 1530: }
>> 
>> I just saw that `_f` is a `short`, which I think is 16 bits, right? And the cast to `uint` would mean we take 32 bits. That looks a bit off, but maybe it is not. Can you explain, and maybe also put a comment in the code for that?
>
> This is to comply with Node::hash signature which returns uint value

But does that not mean that we have a 4-byte load, but the end of the object already happens after 2 bytes? If so, what are those 2 extra bytes? Is that safe and correct?

>> test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 494:
>> 
>>> 492:         assertResult(fma(valueOf(1.0f), valueOf(2.0f), valueOf(3.0f)).floatValue(), 1.0f * 2.0f + 3.0f, "testFMAConstantFolding");
>>> 493:     }
>>> 494: }
>> 
>> I am missing constant folding tests with `shortBitsToFloat16` etc.
>
> Added a few test points for the same

Oh, I was more hoping for a separate test with `shortBitsToFloat16`, not mixed in with `fma`.
And what about constant folding tests for `float16ToRawShortBits`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1888887926
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1888885400
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1888869947
PR Review Comment: https://git.openjdk.org/jdk/pull/22754#discussion_r1888875984