Michael Weiser michael.weiser@gmx.de writes:
Subject: [PATCH 1/4] Mamone's unmodified patch
Hi, I've merged this, but I have a couple of comments and questions.
--- a/Makefile.in +++ b/Makefile.in @@ -616,6 +616,7 @@ distdir: $(DISTFILES) set -e; for d in sparc32 sparc64 x86 \ x86_64 x86_64/aesni x86_64/sha_ni x86_64/fat \ arm arm/neon arm/v6 arm/fat \
arm64 arm64/v8 \
Why the name "v8" for the directory, aren't arm64 and v8 more or less synonyms? I think it would make more sense with a name connected to the extension needed for the pmull instructions.
--- /dev/null +++ b/arm64/v8/gcm-hash.asm @@ -0,0 +1,343 @@
+C common macros: +.macro PMUL in, param1, param2
- pmull F.1q,\param2().1d,\in().1d
- pmull2 F1.1q,\param2().2d,\in().2d
- pmull R.1q,\param1().1d,\in().1d
- pmull2 R1.1q,\param1().2d,\in().2d
- eor F.16b,F.16b,F1.16b
- eor R.16b,R.16b,R1.16b
+.endm
For consistency, I'd prefer defining all needed macros using m4.
--- a/configure.ac +++ b/configure.ac @@ -81,6 +81,10 @@ AC_ARG_ENABLE(arm-neon, AC_HELP_STRING([--enable-arm-neon], [Enable ARM Neon assembly. (default=auto)]),, [enable_arm_neon=auto])
+AC_ARG_ENABLE(armv8-a-crypto,
- AC_HELP_STRING([--enable-armv8-a-crypto], [Enable Armv8-A Crypto extension. (default=no)]),,
- [enable_armv8_a_crypto=no])
I think this would be more user-friendle without the "a", --enable-armv8-crypto, or --enable-arm64-crypto. Or do you foresee any collision with an incompatible ARMv8-M crypto extension or the like?
- aarch64*)
if test "$enable_armv8_a_crypto" = yes ; thenif test "$ABI" = 64 ; thenCFLAGS="$CFLAGS -Wa,-march=armv8-a+crypto"
(This looks slightly different after merging all the changes).
I think it's unfortunate to have to modify CFLAGS, and in particular using compiler-specific options. Is there any way to use a pseudoop in the .asm file instead, similar to the .fpu neon used in the arm/neon/ files?
One could also consider introducing a separate ASMFLAGS make variable (suggested earlier by Jeffrey Walton, for other reasons).
Regards, /Niels