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

[Emanuel Peter]

On Tue, 27 May 2025 14:11:34 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 with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8352635
>  - Enabling some test points
>  - Adding test points and some re-factoring
>  - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8352635
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8352635
>  - 8352635: Improve inferencing of Float16 operations with constant inputs

@jatin-bhateja That looks very promising, thanks for working on that!

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

> 263:   Node* con_inp = nullptr;
> 264:   Node* var_inp = nullptr;
> 265:   if (Float16NodeFactory::is_float32_binary_oper(in(1)->Opcode())) {

It would be nice if you had a summary here, mentioning what "patterns" you are transforming `from -> to`.

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

> 278:     }
> 279: 
> 280:     if (con_inp && var_inp &&

These are implicit null checks. The style guide does not allow this:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

> Do not use ints or pointers as (implicit) booleans with &&, ||, if, while. Instead, compare explicitly, i.e. if (x != 0) or if (ptr != nullptr), etc.

Suggestion:

    if (con_inp != nullptr &&
        var_inp != nullptr &&

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

> 288:       // If constant lie within Float16 value range, convert it to
> 289:       // a half-float constant.
> 290:       if (StubRoutines::hf2f(StubRoutines::f2hf(conF)) == conF) {

How does this behave with `NaN` values? Do you have a test for that below?

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

> 296:         } else {
> 297:           f16bOp = phase->transform(Float16NodeFactory::make(f32bOp->Opcode(), f32bOp->in(0), new_var_inp, new_con_inp));
> 298:         }

Why is the order important here? A comment could help :)

src/hotspot/share/opto/subnode.cpp line 566:

> 564:   // applicable to other floating point types.
> 565:   // There are no known undefined, unspecified or implimentation specific
> 566:   // behaviors w.r.t to floating point non-pointer subtraction.

That sounds like we are not quite sure "no known" ... problems. Could there be any, or are we sure there are none?

test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 320:

> 318:          res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() / INEXACT_FP16);
> 319:          assertResult(Float.float16ToFloat(res), 32.125f, "testInexactFP16ConstantPatterns");
> 320:     }

Alignment is messed up by one space indentation.

Can you add a comment why we are expecting none of the `HF` ops here? 
Are we expecting any other ops, maybe `F` ops?
It could be good to check for that, so that we are sure that we get anything even close to our expectation.

test/hotspot/jtreg/compiler/c2/irTests/TestFloat16ScalarOperations.java line 363:

> 361:          res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() / EXACT_FP16);
> 362:          assertResult(Float.float16ToFloat(res), 32.125f, "testExactFP16ConstantPatterns");
> 363:     }

Can we have a test that picks a random `FP16` value, and does result verification on it? Because currently, you are testing the new pattern only with a few example values.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24179#pullrequestreview-2874142792
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111290680
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111316864
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111320705
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111334455
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111337686
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111347398
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2111351316