RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Dong Bo
As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.

In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].

This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.

[1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
[2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
[3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
[4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070

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

Commit messages:
 - put all VReduction together
 - put all VLoadStore together
 - 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Changes: https://git.openjdk.java.net/jdk/pull/2273/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2273&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258953
  Stats: 5703 lines in 3 files changed: 3241 ins; 2442 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2273.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2273/head:pull/2273

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Ningsheng Jian-2
On Thu, 28 Jan 2021 01:37:33 GMT, Dong Bo <[hidden email]> wrote:

> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>
> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>
> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>
> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070

I managed to sort all the instructs and compare them with and without the patch. They are general the same except for some trailing whitespaces and typos you mentioned.

src/hotspot/cpu/aarch64/aarch64_neon.ad line 776:

> 774:            as_FloatRegister($vsrc$$reg), 0, 1);
> 775:     __ fadds(as_FloatRegister($dst$$reg),
> 776:              as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));

Remove trailing whitespace.

src/hotspot/cpu/aarch64/aarch64_neon.ad line 880:

> 878:            as_FloatRegister($vsrc$$reg), 0, 1);
> 879:     __ faddd(as_FloatRegister($dst$$reg),
> 880:              as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));

Trailing whitespace.

src/hotspot/cpu/aarch64/aarch64_neon.ad line 800:

> 798:            as_FloatRegister($vsrc$$reg), 0, 1);
> 799:     __ fadds(as_FloatRegister($dst$$reg),
> 800:              as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));

trailing whitespace

src/hotspot/cpu/aarch64/aarch64_neon.ad line 4936:

> 4934:   format %{ "fsqrt  $dst, $src\t# vector (2D)" %}
> 4935:   ins_encode %{
> 4936:     __ fsqrt(as_FloatRegister($dst$$reg), __ T2D,

trailing whitespace

src/hotspot/cpu/aarch64/aarch64_neon.ad line 900:

> 898:            as_FloatRegister($vsrc$$reg), 0, 1);
> 899:     __ fmuld(as_FloatRegister($dst$$reg),
> 900:              as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));

trailing whitespace?

src/hotspot/cpu/aarch64/aarch64_neon.ad line 5022:

> 5020:   match(Set dst (OrV src1 src2));
> 5021:   ins_cost(INSN_COST);
> 5022:   format %{ "orr  $dst,$src1,$src2\t# vector (8B)" %}

Good catch for this typo!

src/hotspot/cpu/aarch64/aarch64_neon.ad line 5937:

> 5935:              as_FloatRegister($src$$reg));
> 5936:     } else {
> 5937:       __ usra(as_FloatRegister($dst$$reg), __ T4H,

I see you have fixed this typo, from ushr to usra. I presume original version generates wrong code and produces wrong results for specific case? If so, do you think it deserves a separate fix, e.g. for jdk16?

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

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Andrew Haley
On 1/28/21 10:40 AM, Ningsheng Jian wrote:
> I see you have fixed this typo, from ushr to usra. I presume original version generates wrong code and produces wrong results for specific case? If so, do you think it deserves a separate fix, e.g. for jdk16?

It does. This patch should change nothing at all, except moving
text from A to B.

--
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Dong Bo
In reply to this post by Ningsheng Jian-2
On Thu, 28 Jan 2021 10:37:47 GMT, Ningsheng Jian <[hidden email]> wrote:

>> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>>
>> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
>> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
>> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>>
>> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>>
>> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
>> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
>> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
>> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070
>
> I managed to sort all the instructs and compare them with and without the patch. They are general the same except for some trailing whitespaces and typos you mentioned.

> _Mailing list message from [Andrew Haley](mailto:[hidden email]) on [hotspot-dev](mailto:[hidden email]):_
>
> On 1/28/21 10:40 AM, Ningsheng Jian wrote:
>
> > I see you have fixed this typo, from ushr to usra. I presume original version generates wrong code and produces wrong results for specific case? If so, do you think it deserves a separate fix, e.g. for jdk16?
>
> It does. This patch should change nothing at all, except moving
> text from A to B.
>
> --
> Andrew Haley (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

@nsjian @theRealAph Thank you for the comments. I'll raise a seperate PR to fix this right now.

BTW, since Andrew says we should change nothing at all in this move, do you think we should also do the things below in separtate PRs?
1. fix the typo of `vor8B`.
2. supporting vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`.

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

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad [v2]

Dong Bo
In reply to this post by Dong Bo
> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>
> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>
> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>
> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070

Dong Bo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:

  8258953: AArch64: move NEON instructions to aarch64_neon.ad

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

Changes: https://git.openjdk.java.net/jdk/pull/2273/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2273&range=01
  Stats: 5661 lines in 3 files changed: 3216 ins; 2435 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2273.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2273/head:pull/2273

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad [v2]

Dong Bo
In reply to this post by Dong Bo
On Thu, 28 Jan 2021 12:09:36 GMT, Dong Bo <[hidden email]> wrote:

>> I managed to sort all the instructs and compare them with and without the patch. They are general the same except for some trailing whitespaces and typos you mentioned.
>
>> _Mailing list message from [Andrew Haley](mailto:[hidden email]) on [hotspot-dev](mailto:[hidden email]):_
>>
>> On 1/28/21 10:40 AM, Ningsheng Jian wrote:
>>
>> > I see you have fixed this typo, from ushr to usra. I presume original version generates wrong code and produces wrong results for specific case? If so, do you think it deserves a separate fix, e.g. for jdk16?
>>
>> It does. This patch should change nothing at all, except moving
>> text from A to B.
>>
>> --
>> Andrew Haley (he/him)
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> https://keybase.io/andrewhaley
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>
> @nsjian @theRealAph Thank you for the comments. I'll raise a seperate PR to fix this right now.
>
> BTW, since Andrew says we should change nothing at all in this move, do you think we should also do the things below in separtate PRs?
> 1. fix the typo of `vor8B`.
> 2. supporting vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`.

Updated. The whitespaces mentioned are addressed.
The format typo fix in `vor8B` is kept, other instructions are appended to aarch64_neon.ad.

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

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad [v2]

Andrew Haley-2
In reply to this post by Dong Bo
On Sun, 7 Feb 2021 07:57:57 GMT, Dong Bo <[hidden email]> wrote:

>> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>>
>> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
>> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
>> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>>
>> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>>
>> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
>> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
>> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
>> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070
>
> Dong Bo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
>
>   8258953: AArch64: move NEON instructions to aarch64_neon.ad

That looks fine. I haven't been able to check that all this patch does is move code from aarch64.ad to aarch64_neon.ad, but I believe you.

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

Marked as reviewed by aph (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad [v2]

Ningsheng Jian-2
In reply to this post by Dong Bo
On Sun, 7 Feb 2021 07:57:57 GMT, Dong Bo <[hidden email]> wrote:

>> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>>
>> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
>> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
>> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>>
>> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>>
>> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
>> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
>> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
>> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070
>
> Dong Bo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
>
>   8258953: AArch64: move NEON instructions to aarch64_neon.ad

I compared all-ad-src.ad with and without the patch, and it looked good to me.

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

Marked as reviewed by njian (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8258953: AArch64: move NEON instructions to aarch64_neon.ad [v2]

Dong Bo
On Mon, 8 Feb 2021 02:05:59 GMT, Ningsheng Jian <[hidden email]> wrote:

>> Dong Bo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
>>
>>   8258953: AArch64: move NEON instructions to aarch64_neon.ad
>
> I compared all-ad-src.ad with and without the patch, and it looked good to me.

Thanks for the review.

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

PR: https://git.openjdk.java.net/jdk/pull/2273
Reply | Threaded
Open this post in threaded view
|

Integrated: 8258953: AArch64: move NEON instructions to aarch64_neon.ad

Dong Bo
In reply to this post by Dong Bo
On Thu, 28 Jan 2021 01:37:33 GMT, Dong Bo <[hidden email]> wrote:

> As discussed in [1], all NEON instructions should be moved from `aarch64.ad` to `aarch64_neon.ad`.
>
> In the first commit [2] of this PR, the NEON instructions are deleted from `aarch64.ad` and appended to `aarch64_neon.ad`.
> I compared the generated code in `aarch64_neon.ad` with original code in `aarch64.ad`, no suspicious differences found.
> The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].
>
> This also supports vector length 4 for `vsraa8B_imm` and `vsrla8B_imm`, vector length 2 for `vsraa4S_imm` and `vsrla4S_imm`, fixes few typos, e.g. `vor8B`, `vsrla4S_imm`.
>
> [1] https://github.com/openjdk/jdk/pull/1215#issuecomment-728186803
> [2] https://github.com/dgbo/jdk/commit/40cbe99e647cdf93712edf8f77ab3b5b30ea0a95
> [3] https://github.com/dgbo/jdk/commit/695fb8f8ef009b733a8f804e791347f4bfe2572e
> [4] https://github.com/dgbo/jdk/commit/e0c38aa9aaa6af9925a3821328384b1e2b2c2070

This pull request has now been integrated.

Changeset: aa5bc6ed
Author:    Dong Bo <[hidden email]>
Committer: Fei Yang <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/aa5bc6ed
Stats:     5661 lines in 3 files changed: 3216 ins; 2435 del; 10 mod

8258953: AArch64: move NEON instructions to aarch64_neon.ad

Reviewed-by: njian, aph

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

PR: https://git.openjdk.java.net/jdk/pull/2273