Re: [PATCH] RISC-V: Add configure check for Zaamo/Zalrsc assembler support

[Palmer Dabbelt <palmer@dabbelt.com>]

On Wed, 12 Jun 2024 16:20:26 PDT (-0700), Patrick O'Neill wrote:
> Binutils 2.42 and before don't support Zaamo/Zalrsc. Add a configure
> check to prevent emitting Zaamo/Zalrsc in the arch string when the
> assember does not support it.

Should we just rewrite these to A when binutils doesn't support the 
subsets?  That'd avoid a forced binutils bump, but really user should be 
upgrading anyway...  Either way

Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V

though I'm not suer if the configure churn is sane, it looks like a 
version mismatch of some sort.  Hopefully someone who knows those bits 
better can chime in?

> gcc/ChangeLog:
>
> 	* common/config/riscv/riscv-common.cc
> 	  (riscv_subset_list::to_string): Skip zaamo/zalrsc when not
> 	  supported by the assembler.
> 	* config.in: Regenerate.
> 	* configure: Regenerate.
> 	* configure.ac: Add zaamo/zalrsc assmeber check.
>
> Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
> ---
> Tested using newlib rv64gc with binutils tip-of-tree and 2.42.
>
> This results in calls being emitted when compiling for _zaamo_zalrsc
> when the assember does not support these extensions.
>
>> cat amo.c
> void foo (int* bar, int* baz)
> {
>   __atomic_add_fetch(bar, baz, __ATOMIC_RELAXED);
> }
>> gcc -march=rv64id_zaamo_zalrsc -O3 amo.c
> results in:
> foo:
>         sext.w  a1,a1
>         li      a2,0
>         tail    __atomic_fetch_add_4
>
> As a result there are some testsuite failures on zalrsc specific
> testcases and when using an old version of binutils on non-a targets.
> Not a cause for concern imo but worth calling out.
> Also testcases that check for the default isa string will fail with
> the old binutils since zaamo/zalrsc aren't emitted anymore.
> ---
>  gcc/common/config/riscv/riscv-common.cc | 11 +++++++
>  gcc/config.in                           |  6 ++++
>  gcc/configure                           | 41 ++++++++++++++++++++++---
>  gcc/configure.ac                        |  5 +++
>  4 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
> index 78dfd6b1470..1dc1d9904c7 100644
> --- a/gcc/common/config/riscv/riscv-common.cc
> +++ b/gcc/common/config/riscv/riscv-common.cc
> @@ -916,6 +916,7 @@ riscv_subset_list::to_string (bool version_p) const
>    riscv_subset_t *subset;
>
>    bool skip_zifencei = false;
> +  bool skip_zaamo_zalrsc = false;
>    bool skip_zicsr = false;
>    bool i2p0 = false;
>
> @@ -943,6 +944,10 @@ riscv_subset_list::to_string (bool version_p) const
>       a mistake in that binutils 2.35 supports zicsr but not zifencei.  */
>    skip_zifencei = true;
>  #endif
> +#ifndef HAVE_AS_MARCH_ZAAMO_ZALRSC
> +  /* Skip since binutils 2.42 and earlier don't recognize zaamo/zalrsc.  */
> +  skip_zaamo_zalrsc = true;
> +#endif
>
>    for (subset = m_head; subset != NULL; subset = subset->next)
>      {
> @@ -954,6 +959,12 @@ riscv_subset_list::to_string (bool version_p) const
>  	  subset->name == "zicsr")
>  	continue;
>
> +      if (skip_zaamo_zalrsc && subset->name == "zaamo")
> +	continue;
> +
> +      if (skip_zaamo_zalrsc && subset->name == "zalrsc")
> +	continue;
> +
>        /* For !version_p, we only separate extension with underline for
>  	 multi-letter extension.  */
>        if (!first &&
> diff --git a/gcc/config.in b/gcc/config.in
> index e41b6dc97cd..acab3c0f126 100644
> --- a/gcc/config.in
> +++ b/gcc/config.in
> @@ -629,6 +629,12 @@
>  #endif
>
>
> +/* Define if the assembler understands -march=rv*_zaamo_zalrsc. */
> +#ifndef USED_FOR_TARGET
> +#undef HAVE_AS_MARCH_ZAAMO_ZALRSC
> +#endif
> +
> +
>  /* Define if the assembler understands -march=rv*_zifencei. */
>  #ifndef USED_FOR_TARGET
>  #undef HAVE_AS_MARCH_ZIFENCEI
> diff --git a/gcc/configure b/gcc/configure
> index aaf5899cc03..09b794c1225 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -6228,7 +6228,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -6274,7 +6274,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -6298,7 +6298,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -6343,7 +6343,7 @@ else
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -6367,7 +6367,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>      We can't simply define LARGE_OFF_T to be 9223372036854775807,
>      since some C++ compilers masquerading as C compilers
>      incorrectly reject 9223372036854775807.  */
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>    int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
>  		       && LARGE_OFF_T % 2147483647 == 1)
>  		      ? 1 : -1];
> @@ -30820,6 +30820,37 @@ if test $gcc_cv_as_riscv_march_zifencei = yes; then
>
>  $as_echo "#define HAVE_AS_MARCH_ZIFENCEI 1" >>confdefs.h
>
> +fi
> +
> +    { $as_echo "$as_me:${as_lineno-$LINENO}: checking assembler for -march=rv32i_zaamo_zalrsc support" >&5
> +$as_echo_n "checking assembler for -march=rv32i_zaamo_zalrsc support... " >&6; }
> +if ${gcc_cv_as_riscv_march_zaamo_zalrsc+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  gcc_cv_as_riscv_march_zaamo_zalrsc=no
> +  if test x$gcc_cv_as != x; then
> +    $as_echo '' > conftest.s
> +    if { ac_try='$gcc_cv_as $gcc_cv_as_flags -march=rv32i_zaamo_zalrsc -o conftest.o conftest.s >&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
> +	gcc_cv_as_riscv_march_zaamo_zalrsc=yes
> +    else
> +      echo "configure: failed program was" >&5
> +      cat conftest.s >&5
> +    fi
> +    rm -f conftest.o conftest.s
> +  fi
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_as_riscv_march_zaamo_zalrsc" >&5
> +$as_echo "$gcc_cv_as_riscv_march_zaamo_zalrsc" >&6; }
> +if test $gcc_cv_as_riscv_march_zaamo_zalrsc = yes; then
> +
> +$as_echo "#define HAVE_AS_MARCH_ZAAMO_ZALRSC 1" >>confdefs.h
> +
>  fi
>
>      ;;
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index f8d67efeb98..c54748cd9aa 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -5452,6 +5452,11 @@ configured with --enable-newlib-nano-formatted-io.])
>        [-march=rv32i_zifencei2p0],,,
>        [AC_DEFINE(HAVE_AS_MARCH_ZIFENCEI, 1,
>  		 [Define if the assembler understands -march=rv*_zifencei.])])
> +    gcc_GAS_CHECK_FEATURE([-march=rv32i_zaamo_zalrsc support],
> +      gcc_cv_as_riscv_march_zaamo_zalrsc,
> +      [-march=rv32i_zaamo_zalrsc],,,
> +      [AC_DEFINE(HAVE_AS_MARCH_ZAAMO_ZALRSC, 1,
> +		 [Define if the assembler understands -march=rv*_zaamo_zalrsc.])])
>      ;;
>      loongarch*-*-*)
>      gcc_GAS_CHECK_FEATURE([.dtprelword support],