Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258953: AArch64: move NEON instructions to aarch64_neon.ad #2273

Closed
wants to merge 1 commit into from

Conversation

@dgbo
Copy link
Member

@dgbo dgbo commented Jan 28, 2021

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] #1215 (comment)
[2] dgbo@40cbe99
[3] dgbo@695fb8f
[4] dgbo@e0c38aa


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258953: AArch64: move NEON instructions to aarch64_neon.ad

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2273/head:pull/2273
$ git checkout pull/2273

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 28, 2021

👋 Welcome back dongbo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jan 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@dgbo The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@dgbo
Copy link
Member Author

@dgbo dgbo commented Jan 28, 2021

/label add hotspot-dev

@openjdk openjdk bot added the hotspot label Jan 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@dgbo
The hotspot label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Webrevs

Copy link

@nsjian nsjian left a comment

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.

__ ins(as_FloatRegister($tmp$$reg), __ S,
as_FloatRegister($vsrc$$reg), 0, 1);
__ fadds(as_FloatRegister($dst$$reg),
as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));
Copy link

@nsjian nsjian Jan 28, 2021

Remove trailing whitespace.

__ ins(as_FloatRegister($tmp$$reg), __ D,
as_FloatRegister($vsrc$$reg), 0, 1);
__ faddd(as_FloatRegister($dst$$reg),
as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));
Copy link

@nsjian nsjian Jan 28, 2021

Trailing whitespace.

__ ins(as_FloatRegister($tmp$$reg), __ S,
as_FloatRegister($vsrc$$reg), 0, 1);
__ fadds(as_FloatRegister($dst$$reg),
as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));
Copy link

@nsjian nsjian Jan 28, 2021

trailing whitespace

match(Set dst (SqrtVD src));
format %{ "fsqrt $dst, $src\t# vector (2D)" %}
ins_encode %{
__ fsqrt(as_FloatRegister($dst$$reg), __ T2D,
Copy link

@nsjian nsjian Jan 28, 2021

trailing whitespace

__ ins(as_FloatRegister($tmp$$reg), __ D,
as_FloatRegister($vsrc$$reg), 0, 1);
__ fmuld(as_FloatRegister($dst$$reg),
as_FloatRegister($dst$$reg), as_FloatRegister($tmp$$reg));
Copy link

@nsjian nsjian Jan 28, 2021

trailing whitespace?

n->as_Vector()->length_in_bytes() == 8);
match(Set dst (OrV src1 src2));
ins_cost(INSN_COST);
format %{ "orr $dst,$src1,$src2\t# vector (8B)" %}
Copy link

@nsjian nsjian Jan 28, 2021

Good catch for this typo!

as_FloatRegister($src$$reg),
as_FloatRegister($src$$reg));
} else {
__ usra(as_FloatRegister($dst$$reg), __ T4H,
Copy link

@nsjian nsjian Jan 28, 2021

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?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Mailing list message from Andrew Haley on hotspot-dev:

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

Mailing list message from Andrew Haley on hotspot-compiler-dev:

On 1/28/21 1:41 AM, Dong Bo wrote:

The last two commits just simply move code around in `aarch64_neon.ad` to put related instructions together, i.e. `LoadStore` [3], `Reduction` [4].

I might even do these as separate changes.

--
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

@dgbo
Copy link
Member Author

@dgbo dgbo commented Jan 28, 2021

Mailing list message from Andrew Haley on hotspot-dev:

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.

@dgbo dgbo force-pushed the move_neon_to_aarch64_neon.ad branch from e0c38aa to cb9c0ba Feb 7, 2021
@dgbo
Copy link
Member Author

@dgbo dgbo commented Feb 7, 2021

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

Copy link
Contributor

@theRealAph theRealAph left a comment

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.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 7, 2021

@dgbo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8258953: AArch64: move NEON instructions to aarch64_neon.ad

Reviewed-by: njian, aph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • c5ff454: 8250989: Consolidate buffer allocation code for CDS static/dynamic dumping
  • 0e18634: 8261270: MakeMethodNotCompilableTest fails with -XX:TieredStopAtLevel={1,2,3}

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@nsjian, @theRealAph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Feb 7, 2021
nsjian
nsjian approved these changes Feb 8, 2021
Copy link

@nsjian nsjian left a comment

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

@dgbo
Copy link
Member Author

@dgbo dgbo commented Feb 8, 2021

Thanks for the review.
/integrate

@openjdk openjdk bot added the sponsor label Feb 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 8, 2021

@dgbo
Your change (at version cb9c0ba) is now ready to be sponsored by a Committer.

@RealFYang
Copy link
Member

@RealFYang RealFYang commented Feb 8, 2021

/sponsor

@openjdk openjdk bot closed this Feb 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 8, 2021

@RealFYang @dgbo Since your change was applied there have been 2 commits pushed to the master branch:

  • c5ff454: 8250989: Consolidate buffer allocation code for CDS static/dynamic dumping
  • 0e18634: 8261270: MakeMethodNotCompilableTest fails with -XX:TieredStopAtLevel={1,2,3}

Your commit was automatically rebased without conflicts.

Pushed as commit aa5bc6e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@dgbo dgbo deleted the move_neon_to_aarch64_neon.ad branch Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants