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

@jatin-bhateja Thanks for the updates!

I have a first batch of comments about the test :)

https://github.com/openjdk/jdk/pull/24179#discussion_r2111355331
Here I asked for this:
> And: your pattern matching allows the constant to be lhs or rhs, so you should add corresponding tests.

You commented "Done." underneath. Where did you add these tests exactly? Because I only see these patterns:

        res += Float.floatToFloat16(RANDOM1_VAR.floatValue() + RANDOM2.floatValue());

and not these

        res += Float.floatToFloat16(RANDOM2.floatValue() + RANDOM1_VAR.floatValue());

(except for maybe a single case where one was flipped, see question below)

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

> 61: 
> 62:     private static Generator<Float> genF = G.uniformFloats(0.0f, 70000.0f);
> 63:     private static Generator<Short> genHF = G.uniformFloat16s(Float.floatToFloat16(-2000.0f), Float.floatToFloat16(2000.0f));

Is there a good reason to only take the uniform distribution?

https://github.com/openjdk/jdk/blob/4a491bef6636441f14fc8bbdedf65063fce038bd/test/hotspot/jtreg/compiler/lib/generators/Generators.java#L102-L105

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

> 333:         res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() - INEXACT_FP16);
> 334:         res += Float.floatToFloat16(INEXACT_FP16 * POSITIVE_ZERO_VAR.floatValue());
> 335:         res += Float.floatToFloat16(POSITIVE_ZERO_VAR.floatValue() / INEXACT_FP16);

Why is the mul case flipped here?

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

> 361:     @Check(test="testSNaNFP16ConstantPatterns")
> 362:     public void checkSNaNFP16ConstantPatterns(short actual) throws Exception {
> 363:         TestFramework.deoptimize(TestFloat16ScalarOperations.class.getMethod("testSNaNFP16ConstantPatterns"));

Oh wow, I have never seen this pattern used. Cool idea! Do you know what impact this has on test runtime?

test/hotspot/jtreg/compiler/lib/generators/Generators.java line 622:

> 620: 
> 621:     /**
> 622:      * Fill the array with shorts using the distribution of nextDouble.

Suggestion:

     * Fill the array with shorts using the distribution of the generator.

There are actually a few other cases that seem to be wrong in this file. Would you mind changing them?

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

PR Review: https://git.openjdk.org/jdk/pull/24179#pullrequestreview-2895308949
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125709341
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125713317
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125718345
PR Review Comment: https://git.openjdk.org/jdk/pull/24179#discussion_r2125706470