8316125: Windows call_stub unnecessarily saves xmm16-31 when UseAVX>=3

[djelinski]

Please review this patch that removes saving of xmm16-xmm31 registers from Windows call_stub.

Windows ABI only mandates saving xmm6-xmm15, which we continue to do here.

No new tests. Mach5 tier1-5 builds and tests clean.

---

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-8316125: Windows call_stub unnecessarily saves xmm16-31 when UseAVX>=3 (Enhancement - P4)

Reviewers

• Jorn Vernee (@JornVernee - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)
• Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15688

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back djelinski! 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]

@djelinski 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 pull request command.

[mlbridge]

Webrevs

• 00: Full (6a9343c6)

[JornVernee]

We've discussed this offline. Based on the calling convention described here: https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#callercallee-saved-registers these registers are 'volatile', i.e. not callee saved. That means that in this case, where we have a C function calling into Java, we don't need to save these registers for our C caller.

The relevant parts behind that link copied here (part bolded by me for emphasis):

The x64 ABI considers the registers RAX, RCX, RDX, R8, R9, R10, R11, and XMM0-XMM5 volatile. When present, the upper portions of YMM0-YMM15 and ZMM0-ZMM15 are also volatile. On AVX512VL, the ZMM, YMM, and XMM registers 16-31 are also volatile. When AMX support is present, the TMM tile registers are volatile. Consider volatile registers destroyed on function calls unless otherwise safety-provable by analysis such as whole program optimization.

The x64 ABI considers registers RBX, RBP, RDI, RSI, RSP, R12, R13, R14, R15, and XMM6-XMM15 nonvolatile. They must be saved and restored by a function that uses them.

[JornVernee]

/reviewers 2

[openjdk]

@djelinski 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:

8316125: Windows call_stub unnecessarily saves xmm16-31 when UseAVX>=3

Reviewed-by: jvernee, kvn, sviswanathan

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

• 5350fd6: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
• edcc559: 8316661: CompilerThread leaks CodeBlob memory when dynamically stopping compiler thread in non-product
• 1be3557: 8315721: CloseRace.java#id0 fails transiently on libgraal
• 347bd15: 8315966: Relativize initial_sp in interpreter frames
• b24ad7c: 8316924: java/lang/Thread/virtual/stress/ParkALot.java times out
• ad6df41: 8316710: Exclude java/awt/font/Rotate/RotatedTextTest.java
• 45a145e: 8299915: Remove ArrayAllocatorMallocLimit and associated code
• 50a7a04: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists
• fee9d33: 8293176: SSLEngine handshaker does not send an alert after a bad parameters
• fd52be2: 8316895: SeenThread::print_action_queue called on a null pointer
• ... and 245 more: https://git.openjdk.org/jdk/compare/996b3362bf160fe3b93597fde119c721b6a97cbf...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]

@JornVernee
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).

[dholmes-ora]

This code originally came from the Intel folk:
https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2015-April/017579.html
It would be good if someone from Intel could review the changes. I always like to try and understand why something was originally done the way it was. Perhaps it predates the Windows ABI defining these as volatile?

[djelinski]

Jira suggests that the original submitter of this patch might no longer be around, but any insights from folks more familiar with the problem are welcome.

For what it's worth:

• VS 2015 documentation specifies that XMM16-31 are volatile link
• VS 2015 documentation was released 2015-07, so was not available at the time this patch was merged
• VS 2013 documentation did not mention XMM16-31 at all
  Based on the above, I'd assume that saving XMM16-31 was based on best guess.

Saving of XMM16-31 was added in the second webrev, the first one did not have them. As far as I can tell, the change was not discussed.
https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2015-April/017738.html

[dholmes-ora]

I'm guessing this part of the change description covers it:

The call save regions have been extended for both compilation models to handle their
respective register banks and are working correctly.

[vnkozlov]

@jatin-bhateja or @sviswa7 can you look on this PR?

[djelinski]

@jatin-bhateja or @sviswa7 would one of you be able to find out why the current code saves the high XMM registers?
The code was provided by @mcberg2016, but he's no longer around as far as I can tell.

[sviswa7]

Looks good to me as well.

[djelinski]

/integrate

[openjdk]

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

• 84390dd: 8316658: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java fails intermittently
• 52073b2: 8316994: Avoid modifying ClassLoader and Module objects during -Xshare:dump
• 83c0e45: 8306630: Bump minimum boot jdk to JDK 21
• d3a79b5: 8316371: Open some swing tests 6
• 750da00: 8316743: RISC-V: Change UseVectorizedMismatchIntrinsic option result to warning
• b8cec48: 8315848: G1: Rename rs_ prefix to card_rs in analytics
• 5350fd6: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
• edcc559: 8316661: CompilerThread leaks CodeBlob memory when dynamically stopping compiler thread in non-product
• 1be3557: 8315721: CloseRace.java#id0 fails transiently on libgraal
• 347bd15: 8315966: Relativize initial_sp in interpreter frames
• ... and 251 more: https://git.openjdk.org/jdk/compare/996b3362bf160fe3b93597fde119c721b6a97cbf...master

Your commit was automatically rebased without conflicts.

[openjdk]

@djelinski Pushed as commit 384d2ea.

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