8318562: Computational test more than 2x slower when AVX instructions are used

[sviswa7]

This PR fixes the perf regression seen on AVX for floating point conversions.

In AVX the cvt instructions have three operands cvtxx dst, src1, src2. Where src2 is the one being converted. The dst gets the lower bits as the converted value and upper bits (up to 128) from src1.

The C2 jit uses the cvtxx dst, dst, src2 flavor. Here the problem was due to uninitialized upper bits of the dst XMM register.
Doing an xor dst, dst before the conversion instruction fixes the perf regression.

Perf before the patch on UseAVX=3 platform:
ComputePI.compute_pi_dbl_flt avgt 5 471.875 ± 0.400 ns/op
ComputePI.compute_pi_flt_dbl avgt 5 1877.174 ± 0.557 ns/op
ComputePI.compute_pi_int_dbl avgt 5 655.222 ± 28.082 ns/op
ComputePI.compute_pi_int_flt avgt 5 737.178 ± 0.077 ns/op
ComputePI.compute_pi_long_dbl avgt 5 767.364 ± 0.027 ns/op
ComputePI.compute_pi_long_flt avgt 5 587.854 ± 10.068 ns/op

Perf after the patch on UseAVX=3 platform:
Benchmark Mode Cnt Score Error Units
ComputePI.compute_pi_dbl_flt avgt 5 468.328 ± 0.141 ns/op
ComputePI.compute_pi_flt_dbl avgt 5 435.430 ± 0.259 ns/op
ComputePI.compute_pi_int_dbl avgt 5 424.088 ± 0.050 ns/op
ComputePI.compute_pi_int_flt avgt 5 417.345 ± 0.207 ns/op
ComputePI.compute_pi_long_dbl avgt 5 425.751 ± 0.006 ns/op
ComputePI.compute_pi_long_flt avgt 5 430.199 ± 0.736 ns/op

---

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-8318562: Computational test more than 2x slower when AVX instructions are used (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16701

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16701.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

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.

[sviswa7]

/label hotspot-compiler

[openjdk]

@sviswa7
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 00: Full (3843bf6a)

[vnkozlov]

@sviswa7 thank you for finding the cause! I will test it locally.

[vnkozlov]

I confirmed that this change solved performance issue on machines I tested (old Broadwell and Cascade Lake CPUs).
I am submitting our regular testing for approval.

[sviswa7]

@vnkozlov Thanks a lot!

[jatin-bhateja]

Hi @sviswa7 ,

Thanks for addressing this.

For SSE versions of cvtsi2sd, Bits (MAXVL-1:64) of the corresponding destination register remain unchanged.

For AVX NDS version i.e. vcvtsi2sd, Bits (127:64) of the XMM register destination are copied from the corresponding bits in the first source operand. Bits (MAXVL-1:128) of the destination register are zeroed.

Since all the computation in backend initially happens in logical register and only at retirement backend copies the logical register to architectural register, thus from micro architectural standpoint both the cases will result in emission of extra micro-op, in first case for merging and in second case for copying. Which is why we inject vzeroupper to save merging penalties b/w AVX2/AVX512 and SSE transitions.

Your fix of adding a prior zeroing idiom to break the copying dependency looks ok to me otherwise.

[merykitty]

For cvtss2sd and cvtsd2ss, can we use vcvtss2sd(dst, src, src). This removes the redundant xor on newer machines.

Another solution is to have an inallocatable register that is known to be not stalled. A zero vector register also helps in object creation and calculations that need zeroes in general. However, this fix looks fine to me.

Thanks a lot.

[merykitty]

You don't need TEMP dst, if dst is an alias of src then a destructive xor is not emitted.

[jatin-bhateja]

Without TEMP annotation dst and src may be aliased if src live range does not survives beyond this instruction.

[sviswa7]

I had checked before submitting the PR, the cvt xmm0, xmm0, xmm0 form was slower than xorps xmm1, xmm1 followed by cvt xmm1, xmm1, xmm0.

[vnkozlov]

My tier1-4,xcomp,stress passed.

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

8318562: Computational test more than 2x slower when AVX instructions are used

Reviewed-by: 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 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.

[sviswa7]

Thanks a lot for the reviews @vnkozlov @jatin-bhateja @merykitty.

[sviswa7]

/integrate

[openjdk]

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

• 9b372e2: 8320206: Some intrinsics/stubs missing vzeroupper on x86_64
• 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
• ... and 24 more: https://git.openjdk.org/jdk/compare/0bda467f6e29c866c661e88a76a9fe3efc0a0d19...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sviswa7 Pushed as commit 0881f2b.

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

[merykitty]

@sviswa7 You mean cvt xmm1, xmm0, xmm0 is slower than xorps xmm1, xmm1; cvt xmm1, xmm1, xmm0, right? Since cvt xmm0, xmm0, xmm0 has self dependency so standalone its throughput will be lower than xorps xmm1, xmm1; cvt xmm1, xmm1, xmm0 which does not.

[sviswa7]

/backport jdk21u

[openjdk]

@sviswa7 the backport was successfully created on the branch sviswa7-backport-0881f2b0 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 0881f2b0 from the openjdk/jdk repository.

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

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-0881f2b0:sviswa7-backport-0881f2b0
$ git checkout sviswa7-backport-0881f2b0
# 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-0881f2b0

[sviswa7]

/backport jdk17u-dev

[openjdk]

@sviswa7 Could not automatically backport 0881f2b0 to openjdk/jdk17u-dev due to conflicts in the following files:

• src/hotspot/cpu/x86/macroAssembler_x86.cpp
• src/hotspot/cpu/x86/macroAssembler_x86.hpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk17u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk17u-dev.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-sviswa7-0881f2b0

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 0881f2b0c43870ed10b1166d04cef9832e58629e

# Backport the commit
$ git cherry-pick --no-commit 0881f2b0c43870ed10b1166d04cef9832e58629e
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 0881f2b0c43870ed10b1166d04cef9832e58629e'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk17u-dev with the title Backport 0881f2b0c43870ed10b1166d04cef9832e58629e.