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

[Emanuel Peter]

On Sun, 1 Jun 2025 17:26:07 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> This is a follow-up PR#22755 to improve Float16 operations inferencing.
>> 
>> The existing scheme to detect Float16 operations for some operations is based on pattern matching which expects to receive inputs through ConvHF2F IR, this patch extends matching to accept constant floating point inputs within the Float16 value range.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Extending tests and review resolutions

And some comments about the VM code :)

Looks like we are making good progress here, thanks again for all the work you put in!

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.

src/hotspot/share/opto/convertnode.cpp line 294:

> 292:       // Conditions under which floating point constant can be considered for a pattern match.
> 293:       // 1. Constant must lie within Float16 value range, this will ensure that
> 294:       // we don't unintentially round off float constant to enforce a pattern match.

What do you mean by `enforce a pattern match`?

Are you just trying to say that we have to be careful with the pattern matching here, and we cannot just round off the float constant? Do you have an example where that rounding would lead to issues?

src/hotspot/share/opto/convertnode.cpp line 302:

> 300:       //   results into a quiet NaN but preserves the significand bits of signaling NaN.
> 301:       //   c. Pattern being matched includes a Float to Float16 conversion after binary
> 302:       //   expression, this downcast will still preserve significand bits of binary32 NaN.

Suggestion:

      // 2. If a constant value is one of the valid IEEE 754 binary32 NaN bit patterns
      // then it's safe to consider it for pattern match because of the following reasons:
      //   a. As per section 2.8 of JVMS, Java Virtual Machine does not support
      //   signaling NaN value.
      //   b. Any signaling NaN which takes part in a non-comparison expression
      //   results in a quiet NaN but preserves the significand bits of signaling NaN.
      //   c. The pattern being matched includes a Float to Float16 conversion after binary
      //   expression, this downcast will still preserve the significand bits of binary32 NaN.

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?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24179#pullrequestreview-2895350075
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125731408
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125737423
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125741224
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125743503