[lworld+fp16] RFR: 8330021: AArch64: Add backend support for FP16 add operation [v4]

[Jatin Bhateja]

On Thu, 4 Jul 2024 15:46:05 GMT, Bhavana Kilambi <bkilambi at openjdk.org> wrote:

>> This commit [1] adds initial support for FP16 operations and adds backend support for FP16 add operation for X86. This task adds backend support for scalar and vector FP16 add operation on aarch64.
>> 
>> [1] https://github.com/openjdk/valhalla/commit/f03fb4e4ee4d59ed692d0c26ddce260511f544e7#diff-a799ce8da7f3062bb3699beb65aae504840c649942032e325c2a50f88a2869ad
>
> Bhavana Kilambi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'lworld+fp16'
>  - Remove couple of IR testcases + add --enable-preview flag to tests
>  - Merge branch 'lworld+fp16'
>  - Add a missing return statement in the ad file
>  - 8330021: AArch64: Add backend support for FP16 add operation
>    
>    This commit [1] adds initial support for FP16 operations and adds
>    backend support for FP16 add operation for X86. This task adds backend
>    support for scalar and vector FP16 add operation on aarch64.
>    
>    [1] https://github.com/openjdk/valhalla/commit/f03fb4e4ee4d59ed692d0c26ddce260511f544e7#diff-a799ce8da7f3062bb3699beb65aae504840c649942032e325c2a50f88a2869ad

Hi @Bhavana-Kilambi ,  I have few comments on non-AARCH64 specific code changes.

src/hotspot/share/opto/vectornode.cpp line 595:

> 593:   case Op_ConvF2HF:
> 594:   case Op_ReinterpretS2HF:
> 595:   case Op_ReinterpretHF2S:

ReinterpretHF2S carry a short type, does not qualify as float16 node.

test/hotspot/jtreg/compiler/intrinsics/float16/TestFP16ScalarAdd.java line 58:

> 56:     @Test
> 57:     @IR(applyIfCPUFeature = {"avx512_fp16", "true"}, counts = {IRNode.ADD_HF, "> 0", IRNode.REINTERPRET_S2HF, "> 0", IRNode.REINTERPRET_HF2S, "> 0"})
> 58:     @IR(applyIfCPUFeatureAnd = {"fphp", "true", "asimdhp", "true"}, counts = {IRNode.ADD_HF, "> 0", IRNode.REINTERPRET_S2HF, "> 0", IRNode.REINTERPRET_HF2S, "> 0"})

Please have one IR rules with unified feature list for x86 and aarach64. 
Not sure, but asimdhp is only needed for SIMD instructions I guess.

test/hotspot/jtreg/compiler/intrinsics/float16/TestFP16ScalarAdd.java line 69:

> 67:     @Test
> 68:     @IR(applyIfCPUFeature = {"avx512_fp16", "true"}, failOn = {IRNode.ADD_HF, IRNode.REINTERPRET_S2HF, IRNode.REINTERPRET_HF2S})
> 69:     @IR(applyIfCPUFeatureAnd = {"fphp", "true", "asimdhp", "true"}, failOn = {IRNode.ADD_HF, IRNode.REINTERPRET_S2HF, IRNode.REINTERPRET_HF2S})

Same as above.

test/hotspot/jtreg/compiler/vectorization/TestFloat16VectorSum.java line 60:

> 58:     @Warmup(10000)
> 59:     @IR(applyIfCPUFeatureOr = {"avx512_fp16" , "true", "sve", "true"}, counts = {IRNode.ADD_VHF, " >= 1"})
> 60:     @IR(applyIfCPUFeatureAnd = {"fphp", "true", "asimdhp", "true"}, counts = {IRNode.ADD_VHF, " >= 1"})

Same as above,  multiple IR rules are enforced in cascaded fashion.

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

PR Review: https://git.openjdk.org/valhalla/pull/1096#pullrequestreview-2160289697
PR Review Comment: https://git.openjdk.org/valhalla/pull/1096#discussion_r1666545356
PR Review Comment: https://git.openjdk.org/valhalla/pull/1096#discussion_r1666630243
PR Review Comment: https://git.openjdk.org/valhalla/pull/1096#discussion_r1666630433
PR Review Comment: https://git.openjdk.org/valhalla/pull/1096#discussion_r1666611673