Re: [PATCH, ARM] Select appropriate memcpy implementation for ARMv8-a.

[Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>]

On 02/11/15 16:58, Marcus Shawcroft wrote:
> The newlib configury logic that detects architecture version and chooses
> an appropriate memcpy implementation does not consider ARMv8-a.
> 
> This patch adds configury logic to detect ARMv8-a along with the
> associated changes in Makefile.am and memcpy.
> 
> 
> 2015-11-02  Marcus Shawcroft  <marcus.shawcroft@arm.com>
> 
>         * libc/machine/arm/configure.in: Check for __ARM_ARCH_8A__.
>         * libc/machine/arm/memcpy.S: Handle __ARM_ARCH_8A__.
>         * libc/machine/arm/Makefile.am: Consider HAVE_ARMV8A in memcpy
>         selection.
>         * libc/machine/arm/Makefile.in: Regenerate.
>         * libc/machine/arm/configure: Regenerate.
> 
> OK?

OK.

I see that the automake/autoconf machinery is just following existing
practice.  But I really don't like that, it's very fragile.

I think a better solution would be to have two files in the machine
directory, for example memcpy-generic.c and memcpy.S; both are built
but, depending on the compiler pre-defines only one of them ever
generates a memcpy function for a particular build.  The AArch64 port
does it this way and it's much more flexible.

Could you experiment with a patch to do it that way?

R.

> 
> Cheers
> /Marcus
> 
> 0001-Select-memcpy-for-armv8-a.patch
> 
> 
> diff --git a/newlib/libc/machine/arm/Makefile.am b/newlib/libc/machine/arm/Makefile.am
> index 55bc0a7..0eb1cf1 100644
> --- a/newlib/libc/machine/arm/Makefile.am
> +++ b/newlib/libc/machine/arm/Makefile.am
> @@ -33,6 +33,10 @@ if OPT_SIZE
>  MEMCPY_SRC=
>  MEMCPY_OBJ=
>  else
> +if HAVE_ARMV8A
> +MEMCPY_SRC=memcpy.S
> +MEMCPY_OBJ=$(lpfx)memcpy.o
> +else
>  if HAVE_ARMV7A
>  MEMCPY_SRC=memcpy.S
>  MEMCPY_OBJ=$(lpfx)memcpy.o
> @@ -45,6 +49,7 @@ MEMCPY_SRC=
>  MEMCPY_OBJ=
>  endif !HAVE_ARMV7M
>  endif !HAVE_ARMV7A
> +endif !HAVE_ARMV8A
>  endif !OPT_SIZE
>  
>  lib_a_SOURCES = setjmp.S access.c strcmp.S strcpy.c \
> diff --git a/newlib/libc/machine/arm/Makefile.in b/newlib/libc/machine/arm/Makefile.in
> index 7a3506e..81dea30 100644
> --- a/newlib/libc/machine/arm/Makefile.in
> +++ b/newlib/libc/machine/arm/Makefile.in
> @@ -71,12 +71,14 @@ lib_a_AR = $(AR) $(ARFLAGS)
>  @HAVE_THUMB1_FALSE@am__DEPENDENCIES_1 = $(lpfx)strlen.o
>  @HAVE_THUMB1_TRUE@@OPT_SIZE_TRUE@am__DEPENDENCIES_1 = $(lpfx)strlen.o
>  @HAVE_ARMV7_TRUE@am__DEPENDENCIES_2 = $(lpfx)memchr.o
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@OPT_SIZE_FALSE@am__DEPENDENCIES_3 = $(lpfx)memcpy.o
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@am__DEPENDENCIES_3 =  \
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@	$(lpfx)memcpy.o
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@OPT_SIZE_FALSE@am__objects_1 = lib_a-memcpy.$(OBJEXT)
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@am__objects_1 =  \
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@	lib_a-memcpy.$(OBJEXT)
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@am__DEPENDENCIES_3 = $(lpfx)memcpy.o
> +@HAVE_ARMV7A_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@am__DEPENDENCIES_3 = $(lpfx)memcpy.o
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@am__DEPENDENCIES_3 =  \
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@	$(lpfx)memcpy.o
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@am__objects_1 = lib_a-memcpy.$(OBJEXT)
> +@HAVE_ARMV7A_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@am__objects_1 = lib_a-memcpy.$(OBJEXT)
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@am__objects_1 =  \
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@	lib_a-memcpy.$(OBJEXT)
>  @HAVE_ARMV7_TRUE@am__objects_2 = lib_a-memchr.$(OBJEXT)
>  @HAVE_THUMB1_FALSE@am__objects_3 = lib_a-strlen.$(OBJEXT)
>  @HAVE_THUMB1_TRUE@@OPT_SIZE_TRUE@am__objects_3 =  \
> @@ -226,13 +228,15 @@ noinst_LIBRARIES = lib.a
>  @HAVE_ARMV7_TRUE@MEMCHR_SRC = memchr.S
>  @HAVE_ARMV7_FALSE@MEMCHR_OBJ = 
>  @HAVE_ARMV7_TRUE@MEMCHR_OBJ = $(lpfx)memchr.o
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_FALSE@@OPT_SIZE_FALSE@MEMCPY_SRC = 
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@OPT_SIZE_FALSE@MEMCPY_SRC = memcpy.S
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@MEMCPY_SRC = memcpy.S
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_FALSE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_SRC = 
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_SRC = memcpy.S
> +@HAVE_ARMV7A_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_SRC = memcpy.S
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@MEMCPY_SRC = memcpy.S
>  @OPT_SIZE_TRUE@MEMCPY_SRC = 
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_FALSE@@OPT_SIZE_FALSE@MEMCPY_OBJ = 
> -@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@OPT_SIZE_FALSE@MEMCPY_OBJ = $(lpfx)memcpy.o
> -@HAVE_ARMV7A_TRUE@@OPT_SIZE_FALSE@MEMCPY_OBJ = $(lpfx)memcpy.o
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_FALSE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_OBJ = 
> +@HAVE_ARMV7A_FALSE@@HAVE_ARMV7M_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_OBJ = $(lpfx)memcpy.o
> +@HAVE_ARMV7A_TRUE@@HAVE_ARMV8A_FALSE@@OPT_SIZE_FALSE@MEMCPY_OBJ = $(lpfx)memcpy.o
> +@HAVE_ARMV8A_TRUE@@OPT_SIZE_FALSE@MEMCPY_OBJ = $(lpfx)memcpy.o
>  @OPT_SIZE_TRUE@MEMCPY_OBJ = 
>  lib_a_SOURCES = setjmp.S access.c strcmp.S strcpy.c \
>  	        $(MEMCPY_SRC) $(MEMCHR_SRC) $(STRLEN_SRC) \
> diff --git a/newlib/libc/machine/arm/configure b/newlib/libc/machine/arm/configure
> index 4855100..9f82c7f 100755
> --- a/newlib/libc/machine/arm/configure
> +++ b/newlib/libc/machine/arm/configure
> @@ -567,6 +567,8 @@ LIBOBJS
>  CFLAGS
>  HAVE_ARMV7M_FALSE
>  HAVE_ARMV7M_TRUE
> +HAVE_ARMV8A_FALSE
> +HAVE_ARMV8A_TRUE
>  HAVE_ARMV7A_FALSE
>  HAVE_ARMV7A_TRUE
>  HAVE_ARMV7_FALSE
> @@ -3589,6 +3591,48 @@ else
>  fi
>  
>  
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether armv8a processor is supported" >&5
> +$as_echo_n "checking whether armv8a processor is supported... " >&6; }
> +if ${acnewlib_cv_armv8a_processor+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  cat > conftest.c <<EOF
> +
> +#if defined (__ARM_ARCH_8A__) && defined (__ARM_FEATURE_UNALIGNED)
> +  #define HAVE_ARMV8A
> + #else
> +  #error "ARMV8A is not supported."
> +#endif
> +int main () {
> +  return 0;
> +}
> +EOF
> +if { ac_try='${CC} $CFLAGS $CPPFLAGS -c -o conftest.o conftest.c
> +							1>&5'
> +  { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5
> +  (eval $ac_try) 2>&5
> +  ac_status=$?
> +  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> +  test $ac_status = 0; }; }
> +then
> +  acnewlib_cv_armv8a_processor=yes;
> +else
> +  acnewlib_cv_armv8a_processor=no;
> +fi
> +rm -f conftest*
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $acnewlib_cv_armv8a_processor" >&5
> +$as_echo "$acnewlib_cv_armv8a_processor" >&6; }
> +
> + if test x"$acnewlib_cv_armv8a_processor" = x"yes"; then
> +  HAVE_ARMV8A_TRUE=
> +  HAVE_ARMV8A_FALSE='#'
> +else
> +  HAVE_ARMV8A_TRUE='#'
> +  HAVE_ARMV8A_FALSE=
> +fi
> +
> +
>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether armv7m processor is supported" >&5
>  $as_echo_n "checking whether armv7m processor is supported... " >&6; }
>  if ${acnewlib_cv_armv7m_processor+:} false; then :
> @@ -3837,6 +3881,10 @@ if test -z "${HAVE_ARMV7A_TRUE}" && test -z "${HAVE_ARMV7A_FALSE}"; then
>    as_fn_error $? "conditional \"HAVE_ARMV7A\" was never defined.
>  Usually this means the macro was only invoked conditionally." "$LINENO" 5
>  fi
> +if test -z "${HAVE_ARMV8A_TRUE}" && test -z "${HAVE_ARMV8A_FALSE}"; then
> +  as_fn_error $? "conditional \"HAVE_ARMV8A\" was never defined.
> +Usually this means the macro was only invoked conditionally." "$LINENO" 5
> +fi
>  if test -z "${HAVE_ARMV7M_TRUE}" && test -z "${HAVE_ARMV7M_FALSE}"; then
>    as_fn_error $? "conditional \"HAVE_ARMV7M\" was never defined.
>  Usually this means the macro was only invoked conditionally." "$LINENO" 5
> diff --git a/newlib/libc/machine/arm/configure.in b/newlib/libc/machine/arm/configure.in
> index edf9222..69fbe2b 100644
> --- a/newlib/libc/machine/arm/configure.in
> +++ b/newlib/libc/machine/arm/configure.in
> @@ -111,6 +111,31 @@ rm -f conftest*])
>  
>  AM_CONDITIONAL(HAVE_ARMV7A, test x"$acnewlib_cv_armv7a_processor" = x"yes")
>  
> +dnl Check for whether ARM_ARCH_8A is defined.
> +AC_CACHE_CHECK(whether armv8a processor is supported,
> +	       acnewlib_cv_armv8a_processor, [dnl
> +cat > conftest.c <<EOF
> +
> +#if defined (__ARM_ARCH_8A__) && defined (__ARM_FEATURE_UNALIGNED)
> +  #define HAVE_ARMV8A
> + #else
> +  #error "ARMV8A is not supported."
> +#endif
> +int main () {
> +  return 0;
> +}
> +EOF
> +if AC_TRY_COMMAND([${CC} $CFLAGS $CPPFLAGS -c -o conftest.o conftest.c
> +							1>&AS_MESSAGE_LOG_FD])
> +then
> +  acnewlib_cv_armv8a_processor=yes;
> +else
> +  acnewlib_cv_armv8a_processor=no;
> +fi
> +rm -f conftest*])
> +
> +AM_CONDITIONAL(HAVE_ARMV8A, test x"$acnewlib_cv_armv8a_processor" = x"yes")
> +
>  dnl Check for whether ARM_ARCH_7M is defined.
>  AC_CACHE_CHECK(whether armv7m processor is supported,
>  	       acnewlib_cv_armv7m_processor, [dnl
> diff --git a/newlib/libc/machine/arm/memcpy.S b/newlib/libc/machine/arm/memcpy.S
> index 3997524..b1bab88 100644
> --- a/newlib/libc/machine/arm/memcpy.S
> +++ b/newlib/libc/machine/arm/memcpy.S
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2013 ARM Ltd
> + * Copyright (c) 2013-2015 ARM Ltd
>   * All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
> @@ -40,7 +40,8 @@
>       lib_a-memcpy.o to be generated.
>    */
>  
> -#elif defined (__ARM_ARCH_7A__) && defined (__ARM_FEATURE_UNALIGNED)
> +#elif (__ARM_ARCH >= 7 && __ARM_ARCH_PROFILE == 'A' \
> +       && defined (__ARM_FEATURE_UNALIGNED))
>  #include "memcpy-armv7a.S"
>  
>  #elif defined (__ARM_ARCH_7M__) || defined (__ARM_ARCH_7EM__)
>