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

[Sam James <sam@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 10635 bytes --]

Palmer Dabbelt <palmer@dabbelt.com> writes:

> On Wed, 12 Jun 2024 16:56:09 PDT (-0700), Patrick O'Neill wrote:
>>
>> On 6/12/24 16:49, Sam James wrote:
>>> Palmer Dabbelt <palmer@dabbelt.com> writes:
>>>
>>>> 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?
>>> Your instinct is right!
>>>
>>>>> 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
>>> I think you may be using patched autoconf which fixes
>>> http://bugs.debian.org/742780.
>>>
>>> The fix landed in 2.70: https://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=commit;h=a1d8293f3bfa2516f9a0424e3a6e63c2f8e93c6e.
>>>
>>> Please drop those hunks.
>>
>> I thought I could get away with using the apt autoconf2.69 package
>> directly ;)
>>
>> Thanks. I'll regenerate without those hunks for v2.
>
> FWIW, I just use the distro packages, toss the hunks I don't like, and
> thes re-build things to make sure it doesn't fall over and die.  Just
> don't tell the autoconf people that, I'm sure they'd be horrified ;)

On good Linux distributions, you can do:
$ emerge dev-build/autoconf-vanilla dev-build/automake-vanilla
$ export WANT_AUTOCONF=vanilla-2.69 WANT_AUTOMAKE=vanilla-1.15
$ autoconf --version
autoconf (GNU Autoconf) 2.69
[...]

;)

>
>>
>> Patrick
>>
>>>
>>>
>>>>>   		       && 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],

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]