8320206: Some intrinsics/stubs missing vzeroupper on x86_64

[sviswa7]

The following intrinsics/stubs are missing vzeroupper:
adler32 (since JDK17)
count_positives (since JDK 9)
chacha20 (since JDK 20)
string indexOfChar (since JDK 9)

Adding the missing vzeroupper to avoid AVX-SSE transition penalties.

---

Progress

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

Issue

• JDK-8320206: Some intrinsics/stubs missing vzeroupper on x86_64 (Enhancement - P3)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Jatin Bhateja (@jatin-bhateja - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16678

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@sviswa7 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.

[mlbridge]

Webrevs

• 00: Full (01cea7d3)

[vnkozlov]

@sviswa7 can you remind me where SSE instructions are used which need vzeroupper: intrinsics or code which calls it? I thought we replaced almost all places to use new instructions even for scalar operations in code. And almost all intrinsics have AVX variant.
So which transition we are guarding with vzeroupper?

[sviswa7]

@vnkozlov It is the jitted code to jvm transitions (GC etc) that we are trying to guard. The JVM itself is compiled with base SSE2 as target for 64 bit platforms.

[jatin-bhateja]

Thanks for fixing this @sviswa7, LGTM.

[openjdk]

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

8320206: Some intrinsics/stubs missing vzeroupper on x86_64

Reviewed-by: kvn, jbhateja

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

• 9902d2e: 8315457: Implement JEP 459: String Templates (Second Preview)
• 5522656: 8317834: java/lang/Thread/IsAlive.java timed out
• 9194d2c: 8317357: Update links in building.md to use https rather than http
• 368e4f6: 8315801: [PPC64] JNI code should be more similar to the Panama implementation
• 8ec6b8d: 8319876: Reduce memory consumption of VM_ThreadDump::doit
• bbf52e0: 8319897: Move StackWatermark handling out of LockStack::contains
• 129c470: 8311932: Suboptimal compiled code of nested loop over memory segment
• 369bbec: 8319896: Remove monitor deflation from final audit
• 1588dd9: 8319567: Update java/lang/invoke tests to support vm flags
• 9727f4b: 8320199: Fix HTML 5 errors in java.math.BigInteger
• ... and 21 more: https://git.openjdk.org/jdk/compare/0bda467f6e29c866c661e88a76a9fe3efc0a0d19...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.

[vnkozlov]

Okay. Then if intrinsic stub is called only from compiled code you don't need vzeroupper. You only need it if intrinsics are called from Interpreter or runtime. Which are crc32, crc32c, float16 intrinsics templateInterpreterGenerator.cpp#L472

Arraycopy stubs could be used by VM's runtime, I think. At least they are called from test_arraycopy_func.

I may forgot something about intrinsics. Why we need vzeroupper on all intrinsics exit?

There was actually the issue with vzeroupper called in intrinsics: JDK-8078113.
Then there was JDK-8178811 and followup JDK-8190934

There are a lot of places in VM currently where vzeroupper and it is a mess but we need to clean it up to clear state where we should use it. May be add comments in all places where it is called to state why it is called there.

[sviswa7]

@vnkozlov
Currently in the stock JVM we are generating vzeroupper at the end of a stub and at the end of a C2 jitted method only if it has larger than 128-bit vector instructions. For a C2 jitted method, this could be either due to auto vectorization or due to inline intrinsics. The clear_upper_avx() only marks that the method has larger vectors. The vzeroupper is generated in the method epilog when the marker is found set for the method.

This PR is not deviating from that scheme.

We settled on this scheme based during our discussion on JDK-8178811 and its followup JDK-8190934. The discussion thread for prior is at https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2017-April/026049.html. As part of JDK-8190934 we restricted vzeroupper generation in the epilog of c2 jitted method from always to only when larger vectors are used in the method. This had resolved any over generation of vzeroupper.

[vnkozlov]

I have no question about generating vzeroupper at the end of JITed code because it could be called from Interpreter.
My question only about vzeroupper in intrinsics stubs. Vivek did not answer my question during 8178811 RFR you pointed.
He mentioned Interpreter and C1 (we may need to look on C1 code generation to avoid vzeroupper). But they do not call all intrinsics as I said. And they are not performance critical.

[vnkozlov]

I understand that originally it was good decision to generate vzeroupper in all places where we use avx512 wide instructions (C2 generated SSE instructions at that time). May be it is time to check if C2 still generate SSE now. If not we should fix it so that we can remove vzeroupper in intrinsic stubs used by C2 only.

[iwanowww]

Also, as @cl4es noticed recently, vzeroupper is unconditionally inserted before all CallLeaf/CallLeafNoFP (but not for CallLeafVector) irrespective of what stub is being called. So, when it comes to C2, there are 2 vzeroupper instructions issued.

There's an attempt to detect when vzeroupper is needed (generate_vzeroupper() predicate), but it depends on 2 flags which are set for the whole compilation.

@sviswa7 How expensive vzeroupper is in practice? Does it make sense to introduce a finer-grained heuristic?

[vnkozlov]

Okay. I think I understand what you said. We need vzeroupper in JITed code (because called from Interpreter) if it or intrinsics it calls use avx512 instructions. We don't generate vzeroupper in JITed code epilog if the code does not uses avx512. But intrinsic stub it calls may use avx512 so we delegate vzeroupper generation to stub.
All this is fine but there is duplication: the code may have avx512 instructions, it may call several intrinsics stubs which have them too. Currently we don't check, I think, such duplication but we may need to do that to improve performance.
Please file new RFE.

Saying that I accept your changes and will test it.

[vnkozlov]

My testing tier1-4,xcomp,stress for version v00 passed.

[vnkozlov]

@sviswa7 please, file followup RFEs based on my and Vladimir's Ivanov comments.

[sviswa7]

@vnkozlov @jatin-bhateja @iwanowww Thanks a lot for the reviews. I have filed a RFE (https://bugs.openjdk.org/browse/JDK-8320346).

[sviswa7]

Also, as @cl4es noticed recently, vzeroupper is unconditionally inserted before all CallLeaf/CallLeafNoFP (but not for CallLeafVector) irrespective of what stub is being called. So, when it comes to C2, there are 2 vzeroupper instructions issued.

There's an attempt to detect when vzeroupper is needed (generate_vzeroupper() predicate), but it depends on 2 flags which are set for the whole compilation.

@sviswa7 How expensive vzeroupper is in practice? Does it make sense to introduce a finer-grained heuristic?

vzeroupper is not an expensive instruction but any optimizations is always helpful.
I would like to clarify that both max_vector_size() and clear_upper_avx() used in generate_vzeroupper() are set per compilation and not global. I think that is what you meant as well.

[sviswa7]

/integrate

[openjdk]

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

• b5a7562: 8319985: Delete sun.awt.windows.WToolkit.embedded*() API
• 1fce70b: 8320334: Reflow markdown in testing.md
• 9902d2e: 8315457: Implement JEP 459: String Templates (Second Preview)
• 5522656: 8317834: java/lang/Thread/IsAlive.java timed out
• 9194d2c: 8317357: Update links in building.md to use https rather than http
• 368e4f6: 8315801: [PPC64] JNI code should be more similar to the Panama implementation
• 8ec6b8d: 8319876: Reduce memory consumption of VM_ThreadDump::doit
• bbf52e0: 8319897: Move StackWatermark handling out of LockStack::contains
• 129c470: 8311932: Suboptimal compiled code of nested loop over memory segment
• 369bbec: 8319896: Remove monitor deflation from final audit
• ... and 23 more: https://git.openjdk.org/jdk/compare/0bda467f6e29c866c661e88a76a9fe3efc0a0d19...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sviswa7 Pushed as commit 9b372e2.

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

[sviswa7]

/backport jdk21u

[openjdk]

@sviswa7 the backport was successfully created on the branch sviswa7-backport-9b372e28 in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 9b372e28 from the openjdk/jdk repository.

The commit being backported was authored by Sandhya Viswanathan on 17 Nov 2023 and was reviewed by Vladimir Kozlov and Jatin Bhateja.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git sviswa7-backport-9b372e28:sviswa7-backport-9b372e28
$ git checkout sviswa7-backport-9b372e28
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git sviswa7-backport-9b372e28

[sviswa7]

/backport jdk21u-dev

[openjdk]

@sviswa7 the backport was successfully created on the branch backport-sviswa7-9b372e28 in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 9b372e28 from the openjdk/jdk repository.

The commit being backported was authored by Sandhya Viswanathan on 17 Nov 2023 and was reviewed by Vladimir Kozlov and Jatin Bhateja.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-sviswa7-9b372e28:backport-sviswa7-9b372e28
$ git checkout backport-sviswa7-9b372e28
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-sviswa7-9b372e28