RFR: 8353730: TestSubNodeFloatDoubleNegation.java fails with native Float16 support [v2]

Christian Hagedorn chagedorn at openjdk.org
Thu Apr 10 10:41:36 UTC 2025


On Thu, 10 Apr 2025 09:29:19 GMT, Manuel Hässig <duke at openjdk.org> wrote:

>> Due to insufficient testing on machines supporting FP16 arithmetic in their ISA, I missed that these machines generate two `SUB_FH` nodes and, crucially, an additional `SUB_F` node. We suspect that this comes from some kind of fallback code path ([open issue](https://bugs.openjdk.org/browse/JDK-8353732); also see [discussion in RISC-V PR fixing the same issue](https://github.com/openjdk/jdk/pull/24421#issuecomment-2777755042)).
>> 
>> This PR fixes this issue for all architectures that support FP16 instructions (that I know of), by only matching `SUB_HF` nodes when the CPU supports FP16. The tests for ARM are currently commented out, due to the support for Float16 still being a work in progress (see PR #23748).
>> 
>> I tested the fix using software emulation of an x86_64 CPU with the `avx512_fp16` feature. I also ran the [sanity checks](https://github.com/mhaessig/jdk/actions/runs/14376762241) (the Alpine Linux build fails at `configure`, which is unrelated to this change) as well as tier1 through tier3 and Oracle internal testing.
>
> Manuel Hässig has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8353730-fp16
>  - Fix float16 double negation test

Looks good to me.

test/hotspot/jtreg/compiler/floatingpoint/TestSubNodeFloatDoubleNegation.java line 58:

> 56:     @IR(counts = { IRNode.SUB, "2" }, applyIfPlatform = {"x64", "true"}, applyIfCPUFeature = {"avx512_fp16", "false"})
> 57:     @IR(counts = { IRNode.SUB_HF, "2" }, applyIfPlatform = {"x64", "true"}, applyIfCPUFeature = {"avx512_fp16", "true"})
> 58:     // TODO: uncomment once Float16 support lands in aarch64

Maybe you can also add the actual issue number:
Suggestion:

    // TODO: uncomment once Float16 support lands in aarch64 with JDK-8345125

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24565#pullrequestreview-2756207593
PR Review Comment: https://git.openjdk.org/jdk/pull/24565#discussion_r2037050255


More information about the hotspot-compiler-dev mailing list