Michael Weiser <[email protected]> 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 ; then
> +        if test "$ABI" = 64 ; then
> +          CFLAGS="$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

-- 
Niels Möller. PGP-encrypted email is preferred. Keyid 368C6677.
Internet email is subject to wholesale government surveillance.
_______________________________________________
nettle-bugs mailing list
[email protected]
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Reply via email to