RFR: 8370409: Incorrect computation in Float16 reduction loop

[Emanuel Peter]

On Fri, 24 Oct 2025 14:36:21 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Current floatToFloat16 intrinsic implementation always sign-extends the 16-bit short result to a 32-bit value in anticipation of safe consumption by subsequent integral (comparison) operation[s]. However, the safest way to compare two Float16 values is to use Float16.compare/compareTo method, given that floating point comparisons can also be unordered.
> 
> e.g., both 64512 and -1024 are equivalent bit representations of the Float16 -Inf value, but are not numerically equivalent with integral comparison.
> jshell> Float16.compare(Float16.shortBitsToFloat16((short)-1024), Float16.shortBitsToFlot16((short)64512))
> $3 ==> 0
> 
> In the scalar intrinsic of Float16.add/sub/mul/div/min/max, we always return a boxed value, which is then operated upon by the subsequent Float16 APIs. While Float.floatToFloat16 intrinsic always returns a 'short' value, this is special in the sense that even though the carrier type is 'short' but it encodes an IEEE 754 half precision value, being a short carrier if they get exposed to integral operators, then as per JVM specification, short must be sign-extended before operation.
> 
> Given that our Float16 binary operations inference is based on generic pattern match and is agnostic to how that graph pallet got created, i.e., either through Float16.* APIs or by explicit Float.float16ToFloat/floatToFloat16 operations, hence it's safe to sign-extend the result in all cases.
> 
> Kindly review the patch and share your feedback.
> 
> Best Regards,
> Jatin

@jatin-bhateja Thanks for fixing this! I have a few nits below. I'll run testing once you addressed them :)

test/hotspot/jtreg/compiler/c2/TestFloat16Reduction.java line 33:

> 31:  * @library /test/lib /
> 32:  * @run main/othervm -XX:-TieredCompilation
> 33:  *                   compiler.c2.TestFloat16Reduction

Was the flag required for reproducing the issue?
If it was not required: just remove it
If it was required: add a run without the flag, in addition to a run with the flag.

test/hotspot/jtreg/compiler/c2/TestFloat16Reduction.java line 161:

> 159:         GOLDEN_MAX = MAXReduceLong();
> 160:         GOLDEN_MIN = MINReduceLong();
> 161:     }

A total nit, and optional: you could make the fields static, and just assign values as you declare the fields. That would save you doing it all in the constructor.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27977#pullrequestreview-3397713210
PR Review Comment: https://git.openjdk.org/jdk/pull/27977#discussion_r2476638829
PR Review Comment: https://git.openjdk.org/jdk/pull/27977#discussion_r2476644247