On Tue, 17 Dec 2024 11:05:18 GMT, Jatin Bhateja <jbhat...@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
