Re: [Patch][aarch64] Use IFUNCs to enable LSE instructions in libatomic on aarch64

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

On 31/08/17 18:24, Steve Ellcey wrote:
> On Tue, 2017-08-29 at 12:25 +0100, Szabolcs Nagy wrote:
>> > 
>> > in glibc the hwcap is not used, because it has accesses to
>> > cached dispatch info, but in libatomic using the hwcap
>> > argument is the right way.
> Here is an updated version of the patch to allow aarch64 to use ifuncs
> in libatomic.
> 
> The main difference from the last patch is that the library does not
> access the hwcap value directly but accesses it through the ifunc
> resolver argument.  That means that we no longer need the
> init_cpu_revision static constructor to set a flag that the resolver
> checks, instead the resolver just does a comparision of its incoming
> argument with HWCAP_ATOMICS.
> 

i think this approach is fine.

> This did mean I had to change the prototype for the resolver functions
> in libatomic_i.h to have an argument, which is the way glibc calls
> them.  One complication of this is that the type of the argument can
> differ between platforms and ABIs so I added code to configure.tgt to
> set the type.  I used uint64_t for aarch64 and 'long unsigned int'
> for everything else.  That is not correct for all platforms but at 
> this point no other platforms access the argument so it should not
> matter.  If and when platforms do need to access it they can change
> the type if necessary.
> 

i think this should be improved, see below.

> Steve Ellcey
> sellcey@cavium.com
> 
> 
> 2017-08-31  Steve Ellcey  <sellcey@cavium.com>
> 
> 	* Makefile.am (ARCH_AARCH64_LINUX_LSE): Add IFUNC_OPTIONS and
> 	libatomic_la_LIBADD.
> 	* config/linux/aarch64/host-config.h: New file.
> 	* configure.ac (HWCAP_TYPE): Define.
> 	(AC_CHECK_HEADERS): Check for sys/auxv.h.
> 	(AC_CHECK_FUNCS): Check for getauxval.
> 	(ARCH_AARCH64_LINUX_LSE): New conditional for IFUNC builds.
> 	* configure.tgt (aarch64): Set AARCH and try_ifunc.
> 	(aarch64*-*-linux*) Update config_path.
> 	(aarch64*-*-linux*) Set HWCAP_TYPE.
> 	* libatomic_i.h (GEN_SELECTOR): Add "HWCAP_TYPE hwcap" argument.
> 	* Makefile.in: Regenerate.
> 	* auto-config.h.in: Regenerate.
> 	* configure: Regenerate.
> 
> 
> libatomic.patch
> 
> 
> diff --git a/libatomic/Makefile.am b/libatomic/Makefile.am
> index d731406..a35df1e 100644
> --- a/libatomic/Makefile.am
> +++ b/libatomic/Makefile.am
> @@ -122,6 +122,10 @@ libatomic_la_LIBADD = $(foreach s,$(SIZES),$(addsuffix _$(s)_.lo,$(SIZEOBJS)))
>  
>  ## On a target-specific basis, include alternates to be selected by IFUNC.
>  if HAVE_IFUNC
> +if ARCH_AARCH64_LINUX_LSE
> +IFUNC_OPTIONS	     = -mcpu=thunderx2t99

i'd expect -march=armv8.1-a instead of a particular cpu here.
(and it think this assumes a not very old binutils gas)

> +libatomic_la_LIBADD += $(foreach s,$(SIZES),$(addsuffix _$(s)_1_.lo,$(SIZEOBJS)))
> +endif
>  if ARCH_ARM_LINUX
>  IFUNC_OPTIONS	     = -march=armv7-a -DHAVE_KERNEL64
>  libatomic_la_LIBADD += $(foreach s,$(SIZES),$(addsuffix _$(s)_1_.lo,$(SIZEOBJS)))
> diff --git a/libatomic/configure.ac b/libatomic/configure.ac
> index 023f172..4e06ffe 100644
> --- a/libatomic/configure.ac
> +++ b/libatomic/configure.ac
> @@ -163,6 +163,10 @@ if test -n "$UNSUPPORTED"; then
>    AC_MSG_ERROR([Configuration ${target} is unsupported.])
>  fi
>  
> +# Write out the ifunc resolver arg type.
> +AC_DEFINE_UNQUOTED(HWCAP_TYPE, $HWCAP_TYPE,
> +	[Define type of ifunc resolver function argument.])
> +
>  # Disable fallbacks to __sync routines from libgcc.  Otherwise we'll
>  # make silly decisions about what the cpu can do.
>  CFLAGS="$save_CFLAGS -fno-sync-libcalls $XCFLAGS"
> @@ -171,7 +175,8 @@ CFLAGS="$save_CFLAGS -fno-sync-libcalls $XCFLAGS"
>  AC_STDC_HEADERS
>  ACX_HEADER_STRING
>  GCC_HEADER_STDINT(gstdint.h)
> -AC_CHECK_HEADERS([fenv.h])
> +AC_CHECK_HEADERS([fenv.h sys/auxv.h])
> +AC_CHECK_FUNCS(getauxval)
>  

getauxval is no longer needed.

>  # Check for common type sizes
>  LIBAT_FORALL_MODES([LIBAT_HAVE_INT_MODE])
> @@ -247,6 +252,8 @@ AC_SUBST(LIBS)
>  AC_SUBST(SIZES)
>  
>  AM_CONDITIONAL(HAVE_IFUNC, test x$libat_cv_have_ifunc = xyes)
> +AM_CONDITIONAL(ARCH_AARCH64_LINUX_LSE,
> +	       [expr "$config_path" : ".* linux/aarch64 .*" > /dev/null])

linux/aarch64 seems to be set for all aarch64*-*-linux* targets below.
so why call it _LSE?

>  AM_CONDITIONAL(ARCH_ARM_LINUX,
>  	       [expr "$config_path" : ".* linux/arm .*" > /dev/null])
>  AM_CONDITIONAL(ARCH_I386,
> diff --git a/libatomic/configure.tgt b/libatomic/configure.tgt
> index b8af3ab..0bb5c66 100644
> --- a/libatomic/configure.tgt
> +++ b/libatomic/configure.tgt
> @@ -40,6 +40,14 @@ case "${target_cpu}" in
>    riscv*)		ARCH=riscv ;;
>    sh*)			ARCH=sh ;;
>  
> +  aarch64*)
> +	ARCH=aarch64
> +	case "${target}" in
> +	    aarch64*-*-linux*)
> +		try_ifunc=yes
> +		;;
> +	esac
> +	;;
>    arm*)
>  	ARCH=arm
>  	case "${target}" in
> @@ -109,6 +117,11 @@ fi
>  
>  # Other system configury
>  case "${target}" in
> +  aarch64*-*-linux*)
> +	# OS support for atomic primitives.
> +	config_path="${config_path} linux/aarch64 posix"
> +	;;
> +
>    arm*-*-linux*)
>  	# OS support for atomic primitives.
>  	config_path="${config_path} linux/arm posix"
> @@ -153,3 +166,14 @@ case "${target}" in
>  	UNSUPPORTED=1
>  	;;
>  esac
> +
> +# glibc will pass hwcap to ifunc resolver functions as an argument.
> +# The type may be different on different architectures.
> +case "${target}" in
> +  aarch64*-*-*)
> +	HWCAP_TYPE=uint64_t
> +	;;
> +  *)
> +	HWCAP_TYPE="unsigned long int"
> +	;;

the resolver type on x86_64 is void in principle
(and on other targets it may take multiple args)

so maybe use
 IFUNC_RESOLVER_ARGS='uint64_t hwcap'
and
 IFUNC_RESOLVER_ARGS='void'