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

8253015: Aarch64: Move linux code out from generic CPU feature detection #154

Closed
wants to merge 3 commits into from

Conversation

AntonKozlov
Copy link
Member

@AntonKozlov AntonKozlov commented Sep 14, 2020

Please review a change to remove Linux-specific code from hotspot/cpu/aarch64.

The change is made to prepare for non-linux aarch64 platforms.

vm_version no longer contains any "raw" values that were obtained directly from the platform registers. Some of them may be unavailable on certain architectures, like ctr_el0 is not available on windows [1].

Few opportunities to improve linux code were taken:

  1. HWCAP_ flags now explicitly mapped to CPU_
  2. _dcpop boolean was replaced with CPU_DCPOP
  3. Code generation to get the platform register values was replaced with inline assembly. There is no a code buffer allocation anymore.

Graal-related changes: oracle/graal#2861

Testing:

  • local hotspot tier1
  • looking at detected features
  • looking at debug output for isDcZvaProhibited and zvaLength values in Graal (not committed) -- have not changed
  • looking at the results of ./graal-compiler-micro-benchmarks.jar -p size=128,256,512,1024,2048,4096,8192,16384,32768,65536,131072 ArrayAllocationBenchmark.arrayAllocate (oracle/graal@dcb4506) -- the benchmark is noisy in my environment, probably too many threads are used. Manually compared iterations, there is no regression spotted.

[1] https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-September/009690.html


Progress

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

Issue

  • JDK-8253015: Aarch64: Move linux code out from generic CPU feature detection

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 14, 2020

👋 Welcome back akozlov! 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 Pull request is ready for review label Sep 14, 2020
@openjdk
Copy link

openjdk bot commented Sep 14, 2020

@AntonKozlov The following label will be automatically applied to this pull request: hotspot.

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 (add|remove) "label" command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 14, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 14, 2020

Webrevs

@luhenry
Copy link
Member

luhenry commented Sep 14, 2020

I've verified that this code works for the Windows-AArch64 port with minimal changes to the new vm_version_windows_aarch64.cpp file.

@luhenry
Copy link
Member

luhenry commented Sep 24, 2020

A quick follow-up on getting a review for this patch. We're starting to take dependency on this patch for the Windows-AArch64 and macOS-AArch64 (7bf0aab). /cc @theRealAph

@mlbridge
Copy link

mlbridge bot commented Sep 25, 2020

Mailing list message from Andrew Haley on hotspot-dev:

On 24/09/2020 18:10, Ludovic Henry wrote:

A quick follow-up on getting a review for this patch. We're starting to take dependency on this patch for the
Windows-AArch64 and macOS-AArch64
(https://github.com//pull/212/commits/7bf0aab32fb7ef1659efa09651d59aaed53c31d2). /cc @theRealAph

It's a large and complex patch.

It looks OK, so I have read through and reviewed it, but I haven't tested
it on Linux-AArch64. I assume you've done that.

--
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 bot commented Sep 28, 2020

Mailing list message from Anton Kozlov on hotspot-dev:

Hello Andrew,

On 25.09.2020 15:45, Andrew Haley wrote:

On 24/09/2020 18:10, Ludovic Henry wrote:

A quick follow-up on getting a review for this patch. We're starting to take dependency on this patch for the
Windows-AArch64 and macOS-AArch64
(https://github.com//pull/212/commits/7bf0aab32fb7ef1659efa09651d59aaed53c31d2). /cc @theRealAph

It's a large and complex patch.

It looks OK, so I have read through and reviewed it, but I haven't tested
it on Linux-AArch64. I assume you've done that.

Sorry, do you refer to JDK-8248238 (Implementation of JEP: Windows AArch64) or
to JDK-8253015 (Aarch64: Move linux code out from generic CPU feature
detection)? Because first one was reviewed, but the second one (that is
included in the former) was not.

The intention of this CPU feature detection patch is to do a preparation and
make Windows/AArch64 patch more simple for review.

Could you and community review this (cpu feature detection) patch as well?

Thanks,
Anton

@openjdk
Copy link

openjdk bot commented Sep 28, 2020

@AntonKozlov 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 more details.

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

8253015: Aarch64: Move linux code out from generic CPU feature detection

Reviewed-by: 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 191 new commits pushed to 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 (@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 Pull request is ready to be integrated label Sep 28, 2020
@AntonKozlov
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 28, 2020
@openjdk
Copy link

openjdk bot commented Sep 28, 2020

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

@mlbridge
Copy link

mlbridge bot commented Sep 28, 2020

Mailing list message from Andrew Haley on hotspot-dev:

On 28/09/2020 12:31, Anton Kozlov wrote:

Sorry, do you refer to JDK-8248238 (Implementation of JEP: Windows AArch64) or
to JDK-8253015 (Aarch64: Move linux code out from generic CPU feature
detection)? Because first one was reviewed, but the second one (that is
included in the former) was not.

That's very weird. One of the nice things about the Github web
interface is that when you review a change, the "Approve" button is
right next to the changeset on the page. I'm pretty sure I remember
approving it.

The intention of this CPU feature detection patch is to do a
preparation and make Windows/AArch64 patch more simple for review.
Could you and community review this (cpu feature detection) patch as
well?

Sure, done. I hope it sticks this time.

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

@theRealAph
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Sep 28, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 28, 2020
@openjdk
Copy link

openjdk bot commented Sep 28, 2020

@theRealAph @AntonKozlov Since your change was applied there have been 191 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit ec9bee6.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
3 participants