RFR: 8352635: Improve inferencing of Float16 operations with constant inputs [v5]

[Emanuel Peter]

On Wed, 4 Jun 2025 06:12:41 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Extending tests and review resolutions
>
> src/hotspot/share/opto/convertnode.cpp line 281:
> 
>> 279:       conF = binopF->in(2);
>> 280:       varS = binopF->in(1)->in(1);
>> 281:     }
> 
> Suggestion:
> 
>   if (Float16NodeFactory::is_float32_binary_oper(in(1)->Opcode())) {
>     Node* binopF = in(1);
>     // Check if the incoming binary operation has one floating point constant
>     // input and the other input is a half precision to single precision upcasting node.
>     // We land here because a prior HalfFloat to Float conversion promotes
>     // an integral constant holding Float16 value to a floating point constant.
>     // i.e. ConvHF2F ConI(short) => ConF
>     Node* conF = nullptr;
>     Node* varS = nullptr;
>     if (binopF->in(1)->is_Con() && binopF->in(2)->Opcode() == Op_ConvHF2F) {
>       conF = binopF->in(1);
>       varS = binopF->in(2)->in(1);
>     } else if (binopF->in(2)->is_Con() &&  binopF->in(1)->Opcode() == Op_ConvHF2F) {
>       conF = binopF->in(2);
>       varS = binopF->in(1)->in(1);
>     }
> 
> I think it is better to have the variables just before they are assigned. They are not needed in the scope outside the if at the top here anyway.

You make it sound like this is the only way we get here:

    // We land here because a prior HalfFloat to Float conversion promotes
    // an integral constant holding Float16 value to a floating point constant.
    // i.e. ConvHF2F ConI(short) => ConF

Could this pattern not be created directly with Java code? So maybe rephrase it to "For example, e.g."?

> src/hotspot/share/opto/convertnode.cpp line 304:
> 
>> 302:       //   expression, this downcast will still preserve significand bits of binary32 NaN.
>> 303:       bool isnan = ((*reinterpret_cast<jint*>(&con) & 0x7F800000) == 0x7F800000) &&
>> 304:                    ((*reinterpret_cast<jint*>(&con) & 0x7FFFFF) != 0);
> 
> Why are you hand-crafting this check here? Is there not some predefined function to do this check?

Does `g_isnan` not work here? If not, add a comment why :)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125733195
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125753763