Skip to content

8348561: Add aarch64 intrinsics for ML-DSA #23300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

8348561: Add aarch64 intrinsics for ML-DSA #23300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e40e260
8348561: Add aarch64 intrinsics for ML-DSA
ferakocz Jan 10, 2025
eb7cc43
fixing whitespace errors
ferakocz Jan 27, 2025
3615163
Use SHA3Parallel for matrix generation
ferakocz Jan 30, 2025
5630fd1
merging master
ferakocz Jan 30, 2025
9f7c4a2
removed debugging code
ferakocz Feb 3, 2025
9a3a944
Adding comments + some code reorganization
ferakocz Feb 4, 2025
28b1cf7
renaming a couple of functions
ferakocz Feb 10, 2025
bc1e089
Fixed copyright years
ferakocz Feb 20, 2025
74c1c28
Added comments suggested by Andrew Dinn
ferakocz Feb 20, 2025
54373d5
Accepting suggested change from Andrew Dinn
ferakocz Feb 20, 2025
62d2a3d
Changed aarch64-asmtest.py as suggested by Bhavana-Kilambi
ferakocz Feb 26, 2025
aa0570d
Added more comments, mainly as suggested by Andrew Dinn
ferakocz Feb 26, 2025
d82dfb2
Merged master.
ferakocz Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/hotspot/cpu/aarch64/assembler_aarch64.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2024, Red Hat Inc. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -2552,6 +2552,11 @@ template<typename R, typename... Rx>
ldst_sstr(T, index, a, op1, op2, Vt, Vt2, Vt3, Vt4); \
}

INSN1(ld1, 0b001101010, 0b0000);
INSN2(ld2, 0b001101011, 0b0000);
INSN3(ld3, 0b001101010, 0b0010);
INSN4(ld4, 0b001101011, 0b0010);

INSN1(st1, 0b001101000, 0b0000);
INSN2(st2, 0b001101001, 0b0000);
INSN3(st3, 0b001101000, 0b0010);
Expand Down Expand Up @@ -2586,6 +2591,7 @@ template<typename R, typename... Rx>
void NAME(FloatRegister Vd, SIMD_Arrangement T, FloatRegister Vn, FloatRegister Vm) { \
guarantee(T != T1Q && T != T1D, "incorrect arrangement"); \
if (!acceptT2D) guarantee(T != T2D, "incorrect arrangement"); \
if (opc2 == 0b101101) guarantee(T != T8B && T != T16B, "incorrect arrangement"); \
starti; \
f(0, 31), f((int)T & 1, 30), f(opc, 29), f(0b01110, 28, 24); \
f((int)T >> 1, 23, 22), f(1, 21), rf(Vm, 16), f(opc2, 15, 10); \
Expand All @@ -2609,6 +2615,8 @@ template<typename R, typename... Rx>
INSN(minv, 0, 0b011011, false); // accepted arrangements: T8B, T16B, T4H, T8H, T2S, T4S
INSN(smaxp, 0, 0b101001, false); // accepted arrangements: T8B, T16B, T4H, T8H, T2S, T4S
INSN(sminp, 0, 0b101011, false); // accepted arrangements: T8B, T16B, T4H, T8H, T2S, T4S
INSN(sqdmulh,0, 0b101101, false); // accepted arrangements: T4H, T8H, T2S, T4S
Copy link
Contributor

@Bhavana-Kilambi Bhavana-Kilambi Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, not a comment on the algorithm itself but you might have to add these new instructions in the gtest for aarch64 here - test/hotspot/gtest/aarch64/aarch64-asmtest.py and use this file to generate test/hotspot/gtest/aarch64/asmtest.out.h which would contain these newly added instructions.

Copy link
Contributor Author

@ferakocz ferakocz Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried that, but the python script (actually the as command that it started) threw error messages:

aarch64ops.s:338:24: error: index must be a multiple of 8 in range [0, 32760].
prfm PLDL1KEEP, [x15, 43]
^
aarch64ops.s:357:20: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
sub x1, x10, x23, sxth #2
^
aarch64ops.s:359:20: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
add x11, x21, x5, uxtb #3
^
aarch64ops.s:360:22: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
adds x11, x17, x17, uxtw #1
^
aarch64ops.s:361:20: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
sub x11, x0, x15, uxtb #1
^
aarch64ops.s:362:19: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
subs x7, x1, x0, sxth #2
^
This is without any modifications from what is in the master branch currently.

Copy link
Contributor

@Bhavana-Kilambi Bhavana-Kilambi Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have to use an assembler from the latest binutils build (if the system default isn't the latest) and add the path to the assembler in the "AS" variable. Also you can run it something like - python aarch64-asmtest.py | expand > asmtest.out.h. Please let me know if you still face problems.

Copy link
Contributor

@adinn adinn Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferakocz This also really needs addressing before committing the patch. Perhaps @theRealAph can advise on how to circumvent the problems you found when trying to update the python script?

Copy link
Contributor

@theRealAph theRealAph Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have to use an assembler from the latest binutils build (if the system default isn't the latest) and add the path to the assembler in the "AS" variable. Also you can run it something like - python aarch64-asmtest.py | expand > asmtest.out.h. Please let me know if you still face problems.

People have been running this script for a decade now.

Let's look at just one of these:

aarch64ops.s:357:20: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
sub x1, x10, x23, sxth #2

From the AArch64 manual:

SUB (extended register)
SUB <Xd|SP>, <Xn|SP>, {, {#}}

It thinks this is a SUB (shifted register), bit it's really a SUB (extended register).

fedora:aarch64 $ cat t.s
sub x1, x10, x23, sxth #2
fedora:aarch64 $ as t.s
fedora:aarch64 $ objdump -D a.out
Disassembly of section .text:

0000000000000000 <.text>:
   0:	cb37a941 	sub	x1, x10, w23, sxth #2

So perhaps binutils expects w23 here, not x23. But the manual (ARM DDI 0487K.a) says x23 should be just fine, and, what's more, gives the x form preferred status.

Copy link
Contributor

@theRealAph theRealAph Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried it with top-of trunk latest binutils:

fedora:aarch64 $ ~/binutils-gdb-install/bin/as -march=armv9-a+sha3+sve2-bitperm aarch64ops.s
fedora:aarch64 $ ~/binutils-gdb-install/bin/as --version
GNU assembler (GNU Binutils) 2.44.50.20250225

Copy link
Contributor

@theRealAph theRealAph Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha!

aph@Andrews-MacBook-Pro ~ % as t.s   
t.s:1:19: error: expected 'sxtx' 'uxtx' or 'lsl' with optional integer in range [0, 4]
sub x1, x10, x23, sxth #2
                  ^
aph@Andrews-MacBook-Pro ~ % as --version
Apple clang version 16.0.0 (clang-1600.0.26.6)
Target: arm64-apple-darwin24.3.0

Copy link
Contributor Author

@ferakocz ferakocz Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so GNU as is more forgiving than Apple as...

Copy link
Contributor

@theRealAph theRealAph Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did my patch to aarch64-asmtest.py solve the problem?

Copy link
Contributor Author

@ferakocz ferakocz Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried, I just used GNU as.

INSN(shsubv, 0, 0b001001, false); // accepted arrangements: T8B, T16B, T4H, T8H, T2S, T4S

#undef INSN

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/cpu/aarch64/stubDeclarations_aarch64.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
do_arch_blob, \
do_arch_entry, \
do_arch_entry_init) \
do_arch_blob(compiler, 35000 ZGC_ONLY(+5000)) \
do_arch_blob(compiler, 55000 ZGC_ONLY(+5000)) \
do_stub(compiler, vector_iota_indices) \
do_arch_entry(aarch64, compiler, vector_iota_indices, \
vector_iota_indices, vector_iota_indices) \
Expand Down
Loading