8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) #18562

jatin-bhateja · 2024-04-01T12:01:27Z

Summary of changes include with the patch:-

CPUID based feature detection check for Intel APX extension (https://www.intel.com/content/www/us/en/developer/articles/technical/advanced-performance-extensions-apx.html)
Validation during VM initialization for extended GPRs state save / restoration by OS across context switches of java application threads executing JIT compiled code with new APX ISA.

Kindly review and share your feedback.

Best Regards,
Jatin

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) (Sub-task - P4)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to 0881e43c
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18562/head:pull/18562
$ git checkout pull/18562

Update a local copy of the PR:
$ git checkout pull/18562
$ git pull https://git.openjdk.org/jdk.git pull/18562/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18562

View PR using the GUI difftool:
$ git pr show -t 18562

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18562.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-01T12:02:09Z

👋 Welcome back jbhateja! 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 · 2024-04-01T12:02:30Z

@jatin-bhateja 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:

8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX)

Reviewed-by: sviswanathan, kvn

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 224 new commits pushed to the master branch:

8d2f9e5: 8333749: Consolidate ConstantDesc conversion in java.base
a6fc2f8: 8333412: [s390x] Add support for branch on count instruction
cf677c9: 8333823: Update --release 23 symbol information for JDK 23 build 26
18e7d7b: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
c37d02a: 8312412: Uninitialized klassVtable::_verify_count field
17bd483: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
512b2b4: 8330420: Inverted use of DisplayVMOutputToStderr in ostream_exit
8e72d7c: 8320448: Accelerate IndexOf using AVX2
25ad862: 8332161: Test restoring echo in the Console implementation (java.base)
ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
... and 214 more: https://git.openjdk.org/jdk/compare/985b9ce79a2d620a8b8675d1ae6c9730d72a757f...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-01T12:02:45Z

@jatin-bhateja The following labels will be automatically applied to this pull request:

graal
hotspot

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

jatin-bhateja · 2024-04-01T12:03:05Z

/label add hotspot-compiler-dev

openjdk · 2024-04-01T12:03:50Z

@jatin-bhateja
The hotspot-compiler label was successfully added.

steveatgh · 2024-05-23T23:25:18Z

Hi @jatin-bhateja,

Can you merge with the latest since PR #18476 is in now?

Tuning UseAPX enabling condition

mlbridge · 2024-05-31T10:17:24Z

Webrevs

src/hotspot/cpu/x86/vm_version_x86.hpp

src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp

src/hotspot/cpu/x86/vm_version_x86.cpp

sviswa7 · 2024-06-04T23:58:50Z

src/hotspot/cpu/x86/vm_version_x86.cpp


  VM_Version_StubGenerator(CodeBuffer *c) : StubCodeGenerator(c) {}

+  address clear_apx_test_state() {


Why do we need to clear_apx_test_state? r16 onwards are not callee saved. And checking r15 save/restore is not needed so we could remove r15 changes altogether.

Yes, EGPRs are call clobbered registers, but here we are trying to ascertain if their values are preserved across signal handling. Explicit clearing of r16 and r31 during signal handling guarantees that preserved register values post signal handling were re-instantiated by operating system and not because they were not modified externally.

Please, add comment about that.

src/hotspot/cpu/x86/vm_version_x86.cpp

sviswa7 · 2024-06-05T18:27:53Z

/reviewers 2

openjdk · 2024-06-05T18:28:28Z

@sviswa7
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

sviswa7 · 2024-06-05T19:38:17Z

@jatin-bhateja Please ignore my approval above, it was in mistake, I don't know how to undo that. Please do look into the review comments/suggestions above.

jatin-bhateja · 2024-06-06T19:27:47Z

Hi @vnkozlov , Please let us know if its good to land in 23.

vnkozlov · 2024-06-06T19:39:33Z

Hi @vnkozlov , Please let us know if its good to land in 23.

No, I don't see the urgency. We need extensive testing that everything works with APX.

It is actually good time to push it into JDK 24 to have long testing period before next release. Let us review it and test before integrating.

vnkozlov · 2024-06-06T19:41:25Z

Actually we can't even fully test it until VM start using all registers provided by APX.

vnkozlov · 2024-06-06T19:42:52Z

And we don't have HW currently.

vnkozlov

Few comments

vnkozlov · 2024-06-06T20:12:33Z

src/hotspot/cpu/x86/vm_version_x86.cpp


  VM_Version_StubGenerator(CodeBuffer *c) : StubCodeGenerator(c) {}

+  address clear_apx_test_state() {


Please, add comment about that.

src/hotspot/cpu/x86/vm_version_x86.cpp

src/hotspot/cpu/x86/vm_version_x86.hpp

src/hotspot/cpu/x86/vm_version_x86.cpp

jatin-bhateja · 2024-06-07T02:16:09Z

Actually we can't even fully test it until VM start using all registers provided by APX.

Hi @vnkozlov , EGPR state restoration across signal handling can only be validated after OS support, CPUID and UseAPX flag validation has been done using Intel® Software Development Emulator, other comments addressed.

vnkozlov · 2024-06-07T02:45:01Z

src/hotspot/cpu/x86/vm_version_x86.cpp

+    /* FIXME: Uncomment while integrating JDK-8329032
+    bool save_apx = UseAPX;


What are you missing to uncomment this code?
8329032 is about .ad file changes. It should not affect execution of this code.
You need changes in register_x86.* files and may be somewhere else but you don't need C2 changes for this code to work.

Yes, we already have that in place with #19042, which will be open for review after this patch. I added it in comments since this piece of logic is centered around CPUID feature check and pertinent to this patch.

vnkozlov · 2024-06-07T15:10:57Z

src/hotspot/cpu/x86/vm_version_x86.cpp

 };

+void VM_Version::report_apx_state_restore_warning() {
+  tty->print("warning: Unsuccessful EGPRs state restoration across signal handling, setting UseAPX to false.\n");


This print is fine during development but I would instead save some value in memory to indicate that OS does not save/restore APX. And then check it after we execute this assembler code. Similar how we do that for AVX.
You would not need to do runtime call and this method then.
Note: tty->print() can do "nasty"/unexpected things which you want to avoid.

Hi @vnkozlov , doing a lazy restored state comparison now to align with existing AVX handling.

vnkozlov

Good. Let me test it.

vnkozlov

My testing passed.

jatin-bhateja · 2024-06-09T00:46:10Z

Thanks @vnkozlov and @sviswa7

jatin-bhateja · 2024-06-09T00:46:40Z

/integrate

openjdk · 2024-06-09T00:47:24Z

Going to push as commit a941397.
Since your change was applied there have been 224 commits pushed to the master branch:

8d2f9e5: 8333749: Consolidate ConstantDesc conversion in java.base
a6fc2f8: 8333412: [s390x] Add support for branch on count instruction
cf677c9: 8333823: Update --release 23 symbol information for JDK 23 build 26
18e7d7b: 8333716: Shenandoah: Check for disarmed method before taking the nmethod lock
c37d02a: 8312412: Uninitialized klassVtable::_verify_count field
17bd483: 8333680: com/sun/tools/attach/BasicTests.java fails with "SocketException: Permission denied: connect"
512b2b4: 8330420: Inverted use of DisplayVMOutputToStderr in ostream_exit
8e72d7c: 8320448: Accelerate IndexOf using AVX2
25ad862: 8332161: Test restoring echo in the Console implementation (java.base)
ee82346: 8325984: 4 jcstress tests are failing in Tier6 4 times each
... and 214 more: https://git.openjdk.org/jdk/compare/985b9ce79a2d620a8b8675d1ae6c9730d72a757f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-09T00:47:31Z

@jatin-bhateja Pushed as commit a941397.

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

8329031: CPUID feature detection for APX during VM initialization.

b38266c

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 1, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 1, 2024

Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031

de7d8cb

This was referenced May 1, 2024

8329032: C2 compiler register allocation support for APX EGPRs #19042

Closed

8328998: Encoding support for Intel APX extended general-purpose registers #18476

Closed

Jatin Bhateja added 3 commits May 18, 2024 15:50

Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031

ad39d99

32-bit build fix

e98b5c6

Making UseAPX a boolean flag.

93adcc2

Jatin Bhateja added 4 commits May 25, 2024 22:37

Minor modification in UseAPX flag description

3284fd9

Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8329031

24baa30

Post merge clenups.

97d6538

Update vm_version_x86.cpp

0881e43

Tuning UseAPX enabling condition

jatin-bhateja marked this pull request as ready for review May 31, 2024 10:13

openjdk bot added the rfr Pull request is ready for review label May 31, 2024

sviswa7 approved these changes Jun 5, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 5, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Jun 5, 2024

Review comments resolutions.

b5da093

vnkozlov reviewed Jun 6, 2024

View reviewed changes

Review comments addressed.

68df08c

vnkozlov reviewed Jun 7, 2024

View reviewed changes

Lazy restored state comparison after OS signal handling.

d8fcde9

vnkozlov reviewed Jun 8, 2024

View reviewed changes

vnkozlov approved these changes Jun 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2024

openjdk bot added the integrated Pull request has been integrated label Jun 9, 2024

openjdk bot closed this Jun 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 9, 2024

graalvmbot mentioned this pull request Jun 14, 2024

[GR-54745] Update labsjdk to 24+2-jvmci-b01 oracle/graal#9118

Merged

jatin-bhateja deleted the JDK-8329031 branch August 1, 2024 08:52


		VM_Version_StubGenerator(CodeBuffer *c) : StubCodeGenerator(c) {}

		address clear_apx_test_state() {

		/* FIXME: Uncomment while integrating JDK-8329032
		bool save_apx = UseAPX;

8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) #18562

8329031: CPUID feature detection for Advanced Performance Extensions (Intel® APX) #18562

Uh oh!

Conversation

jatin-bhateja commented Apr 1, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 1, 2024

Uh oh!

openjdk bot commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 1, 2024

Uh oh!

jatin-bhateja commented Apr 1, 2024

Uh oh!

openjdk bot commented Apr 1, 2024

Uh oh!

steveatgh commented May 23, 2024

Uh oh!

mlbridge bot commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sviswa7 commented Jun 5, 2024

Uh oh!

openjdk bot commented Jun 5, 2024

Uh oh!

sviswa7 commented Jun 5, 2024

Uh oh!

jatin-bhateja commented Jun 6, 2024

Uh oh!

vnkozlov commented Jun 6, 2024

Uh oh!

vnkozlov commented Jun 6, 2024

Uh oh!

vnkozlov commented Jun 6, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jatin-bhateja commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin-bhateja Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

jatin-bhateja commented Apr 1, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 1, 2024 •

edited

Loading

mlbridge bot commented May 31, 2024 •

edited

Loading

jatin-bhateja commented Jun 7, 2024 •

edited

Loading

jatin-bhateja Jun 7, 2024 •

edited

Loading