8341052: SHA-512 implementation using SHA-NI #20633

smita-kamath · 2024-08-19T21:34:05Z

Hi, I want to submit an optimization for SHA-512 algorithm using SHA instructions (sha512msg1, sha512msg2 and sha512rnds2) . Kindly review the code and provide feedback. Thank you.

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-8341052: SHA-512 implementation using SHA-NI (Enhancement - P4)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer) Review applies to 85c1aea9
Srinivas Vamsi Parasa (@vamsi-parasa - Author) Review applies to 85c1aea9
Jatin Bhateja (@jatin-bhateja - Reviewer)
Anthony Scarpino (@ascarpino - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20633

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-19T21:34:57Z

👋 Welcome back svkamath! 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-08-19T21:35:23Z

@smita-kamath 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:

8341052: SHA-512 implementation using SHA-NI

Reviewed-by: jbhateja, ascarpino, sviswanathan, sparasa

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

18b55ce: 8342653: Fix minor doc issues in AnnotatedElement
153ad91: 8338126: C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2
80ec552: 8328528: C2 should optimize long-typed parallel iv in an int counted loop
330f2b5: 8342295: compiler/jvmci/TestJVMCISavedProperties.java fails due to garbage in output
1f35748: 8342102: ZGC: Optimize copy constructors in ZPhysicalMemory
66ddaaa: 8340241: RISC-V: Returns mispredicted
07f550b: 8340818: Add a new jtreg test root to test the generated documentation
27ef6c9: 8341470: BigDecimal.stripTrailingZeros() optimization
5d5d88a: 8339570: Add Tidy build support for JDK tests
239d84a: 8342578: GHA: RISC-V: Bootstrap using Debian snapshot is still failing
... and 50 more: https://git.openjdk.org/jdk/compare/b4ab290fd7c3d914154755a1539b48ba33338c26...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.

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 (@sviswa7, @jatin-bhateja, @ascarpino) 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 · 2024-08-19T21:35:46Z

@smita-kamath 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.

openjdk · 2024-09-12T16:14:31Z

@smita-kamath this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout sha-512
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mlbridge · 2024-09-30T15:37:05Z

Webrevs

sviswa7 · 2024-10-04T18:27:12Z

The line 1306 in vm_version_x86.cpp needs to be changed from:
if (UseSHA && supports_avx2() && supports_bmi2()) {
to
if (UseSHA && supports_avx2() && (supports_bmi2() || supports_sha512())) {

sviswa7 · 2024-10-04T17:21:24Z

src/hotspot/cpu/x86/stubRoutines_x86.cpp

@@ -443,4 +443,5 @@ ATTRIBUTE_ALIGNED(64) const julong StubRoutines::x86::_k512_W[] =
    0x4cc5d4becb3e42b6ULL, 0x597f299cfc657e2aULL,
    0x5fcb6fab3ad6faecULL, 0x6c44198c4a475817ULL,
 };
+


Extra blank line could be removed.

sviswa7 · 2024-10-04T18:23:00Z

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

+  assert(VM_Version::supports_avx2() || VM_Version::supports_sha512(), "");
  assert(VM_Version::supports_bmi2(), "");


The SHA512 algorithm is also using AVX2 instructions but doesn't need bmi2.
So this needs to be changed to:
assert(VM_Version::supports_avx2(), "");
assert(VM_Version::supports_bmi2() || VM_Version::supports_sha512(), "");

sviswa7 · 2024-10-04T18:37:17Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

+    Label done_hash, block_loop;
+    address K512_W = StubRoutines::x86::k512_W_addr();
+
+    vbroadcasti128(xmm15, ExternalAddress(StubRoutines::x86::pshuffle_byte_flip_mask_addr_sha512()), rbx);


rbx is save on entry register. Better to use a save on call register as temp, say r10.

sviswa7 · 2024-10-04T20:16:24Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -6762,6 +6762,27 @@ void Assembler::sha256msg2(XMMRegister dst, XMMRegister src) {
  emit_int16((unsigned char)0xCD, (0xC0 | encode));
 }

+void Assembler::sha512msg1(XMMRegister dst, XMMRegister src) {
+  assert(VM_Version::supports_sha512(), "");


For all the new sha512 instructions, this should be:
assert(VM_Version::supports_sha512() && VM_Version::supports_avx(), "");

sviswa7 · 2024-10-04T20:19:35Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -6762,6 +6762,27 @@ void Assembler::sha256msg2(XMMRegister dst, XMMRegister src) {
  emit_int16((unsigned char)0xCD, (0xC0 | encode));
 }

+void Assembler::sha512msg1(XMMRegister dst, XMMRegister src) {
+  assert(VM_Version::supports_sha512(), "");
+  InstructionAttr attributes(AVX_256bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);


For all the new sha512 instructions legacy_mode should be true.

sviswa7 · 2024-10-04T20:38:29Z

src/hotspot/cpu/x86/assembler_x86.cpp

@@ -11672,6 +11693,17 @@ void Assembler::evbroadcasti64x2(XMMRegister dst, Address src, int vector_len) {
  emit_operand(dst, src, 0);
 }

+void Assembler::vbroadcasti128(XMMRegister dst, Address src) {


Please see vbroadcastf128, good to take vector_len as argument here also and have an assert to confirm that it is AVX-256 bit.
Also need:
attributes.set_address_attributes(/* tuple_type / EVEX_T4, / input_size_in_bits */ EVEX_32bit);

sviswa7 · 2024-10-09T00:15:53Z

The following comment is still pending:
The line 1306 in vm_version_x86.cpp needs to be changed from:
if (UseSHA && supports_avx2() && supports_bmi2()) {
to
if (UseSHA && supports_avx2() && (supports_bmi2() || supports_sha512())) {

sviswa7 · 2024-10-09T00:21:59Z

/label hotspot-compiler

openjdk · 2024-10-09T00:23:20Z

@sviswa7
The hotspot-compiler label was successfully added.

sviswa7 · 2024-10-09T20:13:25Z

Looks good to me.

jatin-bhateja · 2024-10-03T05:11:33Z

src/hotspot/cpu/x86/stubRoutines_x86.cpp

@@ -443,4 +443,5 @@ ATTRIBUTE_ALIGNED(64) const julong StubRoutines::x86::_k512_W[] =
    0x4cc5d4becb3e42b6ULL, 0x597f299cfc657e2aULL,
    0x5fcb6fab3ad6faecULL, 0x6c44198c4a475817ULL,
 };
+


Remove this newline.

jatin-bhateja · 2024-10-03T05:11:38Z

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp

+      const XMMRegister msg = xmm0;
+      const XMMRegister state0 = xmm1;
+      const XMMRegister state1 = xmm2;
+      const XMMRegister msgtmp0 = xmm3;
+      const XMMRegister msgtmp1 = xmm4;
+      const XMMRegister msgtmp2 = xmm5;
+      const XMMRegister msgtmp3 = xmm6;
+      const XMMRegister msgtmp4 = xmm7;
+
+      const XMMRegister shuf_mask = xmm8;
+      __ sha512_AVX2(msg, state0, state1, msgtmp0, msgtmp1, msgtmp2, msgtmp3, msgtmp4,
+          buf, state, ofs, limit, rsp, multi_block, shuf_mask);
+  }


Suggested change

const XMMRegister msg = xmm0;

const XMMRegister state0 = xmm1;

const XMMRegister state1 = xmm2;

const XMMRegister msgtmp0 = xmm3;

const XMMRegister msgtmp1 = xmm4;

const XMMRegister msgtmp2 = xmm5;

const XMMRegister msgtmp3 = xmm6;

const XMMRegister msgtmp4 = xmm7;

const XMMRegister shuf_mask = xmm8;

__ sha512_AVX2(msg, state0, state1, msgtmp0, msgtmp1, msgtmp2, msgtmp3, msgtmp4,

buf, state, ofs, limit, rsp, multi_block, shuf_mask);

}

const XMMRegister msg = xmm0;

const XMMRegister state0 = xmm1;

const XMMRegister state1 = xmm2;

const XMMRegister msgtmp0 = xmm3;

const XMMRegister msgtmp1 = xmm4;

const XMMRegister msgtmp2 = xmm5;

const XMMRegister msgtmp3 = xmm6;

const XMMRegister msgtmp4 = xmm7;

const XMMRegister shuf_mask = xmm8;

__ sha512_AVX2(msg, state0, state1, msgtmp0, msgtmp1, msgtmp2, msgtmp3, msgtmp4,

buf, state, ofs, limit, rsp, multi_block, shuf_mask);

}

jatin-bhateja · 2024-10-10T11:52:36Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

+      vpermq(xmm8, xmm4, 0x1b, Assembler::AVX_256bit);//ymm8 = W[20] W[21] W[22] W[23]
+      vpermq(xmm9, xmm3, 0x39, Assembler::AVX_256bit);//ymm9 = W[16] W[19] W[18] W[17]
+      vpblendd(xmm7, xmm8, xmm9, 0x3f, Assembler::AVX_256bit);//ymm7 = W[20] W[19] W[18] W[17]


I assume Algorithm is specifically crafted for 256 bit vectors and with 512 bit extension we may modify it. Do you think we should factor out following pattern and add an alternative implementation for it ?

vpermq(xmm8, xmm4, 0x1b, Assembler::AVX_256bit);//ymm8 = W[20] W[21] W[22] W[23] vpermq(xmm9, xmm3, 0x39, Assembler::AVX_256bit);//ymm9 = W[16] W[19] W[18] W[17] vpblendd(xmm7, xmm8, xmm9, 0x3f, Assembler::AVX_256bit);//ymm7 = W[20] W[19] W[18] W[17]

This is a fixed pattern seen 4 times within computation loop and once outside the loop.
We are permuting two vectors with constant paramutation mask and blending them using immediate mask.
This is a very valid use case for two table permutation instruction VPERMI2Q (available for AVX512VL targets)
We can store permutation pattern outside the loop into a vector and then re-use it within the loop.

We can do this change in a separate PR.

I agree with Smita. The current implementation has a one-to-one correspondence with the ipsec implementation. Any new changes or refactoring could be implemented as a separate PR.

I agree, in principle, any optimization crafted to AVX2 is also applicable to AVX512 target, in future with AVX10.2 (converged ISA) we will have a 256bits flavors of two table permute for non-AVX512 targets, for now AVX-SHA512 is only available on client parts (upcoming Arrow lake) and its ok to follow the IPsec algorithm in toto.

jatin-bhateja · 2024-10-10T12:18:57Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

@@ -1519,5 +1519,183 @@ void MacroAssembler::sha512_AVX2(XMMRegister msg, XMMRegister state0, XMMRegiste
    }
 }

+void MacroAssembler::sha512_update_ni_x1(Register arg_hash, Register arg_msg, Register ofs, Register limit, bool multi_block) {


Please add a comment on this mentioning the source of algorithm.
https://github.com/intel/intel-ipsec-mb/blob/main/lib/avx2_t4/sha512_x1_ni_avx2.asm

vamsi-parasa

This implementation looks good to me.

I went through the implementation of sha512_update_ni_x1. Looked at it line by line and compared it to the ipsec implementation.

Thanks,
Srinivas Vamsi Parasa (Intel)

jatin-bhateja · 2024-10-13T17:05:08Z

src/hotspot/cpu/x86/macroAssembler_x86_sha.cpp

+      vpermq(xmm8, xmm4, 0x1b, Assembler::AVX_256bit);//ymm8 = W[20] W[21] W[22] W[23]
+      vpermq(xmm9, xmm3, 0x39, Assembler::AVX_256bit);//ymm9 = W[16] W[19] W[18] W[17]
+      vpblendd(xmm7, xmm8, xmm9, 0x3f, Assembler::AVX_256bit);//ymm7 = W[20] W[19] W[18] W[17]


I agree, in principle, any optimization crafted to AVX2 is also applicable to AVX512 target, in future with AVX10.2 (converged ISA) we will have a 256bits flavors of two table permute for non-AVX512 targets, for now AVX-SHA512 is only available on client parts (upcoming Arrow lake) and its ok to follow the IPsec algorithm in toto.

smita-kamath · 2024-10-14T20:51:47Z

@ascarpino, I have approvals for this PR. Would it be possible for you to run tests and let me know the results? I appreciate your help. Thanks.

ascarpino

Tier 1-3 passed on windows-x64, linux-x64, and macos-aarch64

smita-kamath · 2024-10-21T15:33:13Z

/integrate

openjdk · 2024-10-21T15:34:18Z

@smita-kamath
Your change (at version af309de) is now ready to be sponsored by a Committer.

sviswa7 · 2024-10-21T15:36:14Z

/sponsor

openjdk · 2024-10-21T15:37:37Z

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

54a744b: 8340553: ZipEntry field validation does not take into account the size of a CEN header
18b55ce: 8342653: Fix minor doc issues in AnnotatedElement
153ad91: 8338126: C2 SuperWord: VectorCastF2HF / vcvtps2ph produces wrong results for vector length 2
80ec552: 8328528: C2 should optimize long-typed parallel iv in an int counted loop
330f2b5: 8342295: compiler/jvmci/TestJVMCISavedProperties.java fails due to garbage in output
1f35748: 8342102: ZGC: Optimize copy constructors in ZPhysicalMemory
66ddaaa: 8340241: RISC-V: Returns mispredicted
07f550b: 8340818: Add a new jtreg test root to test the generated documentation
27ef6c9: 8341470: BigDecimal.stripTrailingZeros() optimization
5d5d88a: 8339570: Add Tidy build support for JDK tests
... and 51 more: https://git.openjdk.org/jdk/compare/b4ab290fd7c3d914154755a1539b48ba33338c26...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-21T15:37:49Z

@sviswa7 @smita-kamath Pushed as commit 18bcbf7.

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

SHA-512 implementation using SHA-NI instructions

Loading
Loading status checks…

61c6639

openjdk bot added the hotspot label Aug 19, 2024

openjdk bot added the merge-conflict label Sep 12, 2024

Merge master

4b6b3e2

openjdk bot removed the merge-conflict label Sep 26, 2024

smita-kamath changed the title ~~SHA-512 implementation using SHA-NI instructions~~ 8341052: SHA-512 implementation using SHA-NI Sep 26, 2024

Updated AMD64.java

16298c5

smita-kamath marked this pull request as ready for review September 30, 2024 15:30

openjdk bot added the rfr label Sep 30, 2024

sviswa7 reviewed Oct 4, 2024

View reviewed changes

Updated code as per review comment & updated test case

afeb502

openjdk bot added the hotspot-compiler label Oct 9, 2024

Addressed a review comment

85c1aea

sviswa7 approved these changes Oct 9, 2024

View reviewed changes

openjdk bot added the ready label Oct 9, 2024

jatin-bhateja reviewed Oct 10, 2024

View reviewed changes

Updated code as per review comments

3cb9175

openjdk bot removed the ready label Oct 10, 2024

vamsi-parasa approved these changes Oct 10, 2024

View reviewed changes

jatin-bhateja approved these changes Oct 13, 2024

View reviewed changes

openjdk bot added the ready label Oct 13, 2024

Merge branch 'master' of https://git.openjdk.org/jdk into sha-512

af309de

graalvmbot mentioned this pull request Oct 18, 2024

[JDK-8341835] Adapt JDK-8341052: SHA-512 implementation using SHA-NI oracle/graal#9918

Merged

ascarpino approved these changes Oct 18, 2024

View reviewed changes

openjdk bot added the sponsor label Oct 21, 2024

openjdk bot added the integrated label Oct 21, 2024

openjdk bot closed this Oct 21, 2024

openjdk bot removed ready rfr sponsor labels Oct 21, 2024

graalvmbot mentioned this pull request Oct 25, 2024

[GR-59375] Update labsjdk to 24+21-jvmci-b01 oracle/graal#9968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341052: SHA-512 implementation using SHA-NI #20633

8341052: SHA-512 implementation using SHA-NI #20633

smita-kamath commented Aug 19, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 19, 2024

openjdk bot commented Aug 19, 2024 •

edited

Loading

openjdk bot commented Aug 19, 2024

openjdk bot commented Sep 12, 2024

mlbridge bot commented Sep 30, 2024 •

edited

Loading

sviswa7 commented Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 Oct 4, 2024

sviswa7 commented Oct 9, 2024

sviswa7 commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

sviswa7 commented Oct 9, 2024

jatin-bhateja Oct 3, 2024

jatin-bhateja Oct 3, 2024

jatin-bhateja Oct 10, 2024 •

edited

Loading

smita-kamath Oct 10, 2024

vamsi-parasa Oct 10, 2024 •

edited

Loading

jatin-bhateja Oct 13, 2024 •

edited

Loading

jatin-bhateja Oct 10, 2024 •

edited

Loading

vamsi-parasa left a comment

jatin-bhateja Oct 13, 2024 •

edited

Loading

smita-kamath commented Oct 14, 2024

ascarpino left a comment

smita-kamath commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

sviswa7 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

		assert(VM_Version::supports_avx2() \|\| VM_Version::supports_sha512(), "");
		assert(VM_Version::supports_bmi2(), "");

8341052: SHA-512 implementation using SHA-NI #20633

8341052: SHA-512 implementation using SHA-NI #20633

Conversation

smita-kamath commented Aug 19, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 19, 2024

openjdk bot commented Aug 19, 2024 • edited Loading

openjdk bot commented Aug 19, 2024

openjdk bot commented Sep 12, 2024

mlbridge bot commented Sep 30, 2024 • edited Loading

Webrevs

sviswa7 commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviswa7 commented Oct 9, 2024

sviswa7 commented Oct 9, 2024

openjdk bot commented Oct 9, 2024

sviswa7 commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jatin-bhateja Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamsi-parasa Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

jatin-bhateja Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

jatin-bhateja Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

vamsi-parasa left a comment

Choose a reason for hiding this comment

jatin-bhateja Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

smita-kamath commented Oct 14, 2024

ascarpino left a comment

Choose a reason for hiding this comment

smita-kamath commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

sviswa7 commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

openjdk bot commented Oct 21, 2024

smita-kamath commented Aug 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 19, 2024 •

edited

Loading

mlbridge bot commented Sep 30, 2024 •

edited

Loading

jatin-bhateja Oct 10, 2024 •

edited

Loading

vamsi-parasa Oct 10, 2024 •

edited

Loading

jatin-bhateja Oct 13, 2024 •

edited

Loading

jatin-bhateja Oct 10, 2024 •

edited

Loading

jatin-bhateja Oct 13, 2024 •

edited

Loading