When 'a' is put into riscv_combine_info, 'a' will only be added into
arch string only if zaamo *AND* zalrsc is there, so zalrsc only won't
trigger that.
On Tue, Jun 18, 2024 at 1:35 PM Patrick O'Neill <patr...@rivosinc.com> wrote:
>
>
>
> On Mon, Jun 17, 2024 at 5:51 PM Kito Cheng <kito.ch...@gmail.com> wrote:
>>
>> Maybe just add 'a' to riscv_combine_info and other logic to keep the
>> same (e.g. keep the logic for skip_zaamo_zalrsc)?
>
>
> I did consider unconditionally upgrading zaamo/zalrsc to ‘a’ (I think that’s
> what you’re suggesting w/ riscv_combine_info).
> That could cause issues if users are trying to compile for a zalrsc-only chip
> with an old version of binutils. If we upgrade zalrsc -> ‘a’ for both cc1 and
> binutils then cc1 will emit amo ops instead of their lr/sc equivalent.
> GCC would end up emitting insns that are illegal for the user-provided -march
> string.
>
> Patrick
>
>>
>> On Tue, Jun 18, 2024 at 8:03 AM Patrick O'Neill <patr...@rivosinc.com> wrote:
>> >
>> > Binutils 2.42 and before don't support Zaamo/Zalrsc. Promote Zaamo/Zalrsc
>> > to
>> > 'a' in the -march string when assembling.
>> >
>> > This change respects Zaamo/Zalrsc when generating code.
>> >
>> > Testcases that check for the default isa string will fail with the old
>> > binutils
>> > since zaamo/zalrsc aren't emitted anymore. All other Zaamo/Zalrsc testcases
>> > pass.
>> >
>> > gcc/ChangeLog:
>> >
>> > * common/config/riscv/riscv-common.cc
>> > (riscv_subset_list::to_string): Add toggle to promote Zaamo/Zalrsc
>> > extensions to 'a'.
>> > (riscv_arch_str): Ditto.
>> > (riscv_expand_arch): Ditto.
>> > (riscv_expand_arch_from_cpu): Ditto.
>> > (riscv_expand_arch_upgrade_exts): New function. Wrapper around
>> > riscv_expand_arch to preserve the function signature.
>> > (riscv_expand_arch_no_upgrade_exts): Ditto
>> > (riscv_expand_arch_from_cpu_upgrade_exts): New function. Wrapper
>> > around
>> > riscv_expand_arch_from_cpu to preserve the function signature.
>> > (riscv_expand_arch_from_cpu_no_upgrade_exts): Ditto.
>> > * config/riscv/riscv-protos.h (riscv_arch_str): Add toggle to
>> > function
>> > prototype.
>> > * config/riscv/riscv-subset.h: Ditto.
>> > * config/riscv/riscv-target-attr.cc (riscv_process_target_attr):
>> > * config/riscv/riscv.cc (riscv_emit_attribute):
>> > (riscv_declare_function_name):
>> > * config/riscv/riscv.h (riscv_expand_arch): Remove.
>> > (riscv_expand_arch_from_cpu): Ditto.
>> > (riscv_expand_arch_upgrade_exts): Add toggle wrapper functions.
>> > (riscv_expand_arch_no_upgrade_exts): Ditto.
>> > (riscv_expand_arch_from_cpu_upgrade_exts): Ditto.
>> > (riscv_expand_arch_from_cpu_no_upgrade_exts): Ditto.
>> > (EXTRA_SPEC_FUNCTIONS): Ditto.
>> > (OPTION_DEFAULT_SPECS): Use non-upgraded march string when
>> > invoking the
>> > compiler.
>> > (ASM_SPEC): Use upgraded march string when invoking the assembler.
>> >
>> > Signed-off-by: Patrick O'Neill <patr...@rivosinc.com>
>> > ---
>> > v3 ChangeLog:
>> > Rebased on non-promoting patch.
>> > Wrap all Zaamo/Zalrsc upgrade code in #ifndef to prevent compiler
>> > warnings about unused/potentially undefined variables.
>> > Silence unused parameter warning with a voidcast.
>> > ---
>> > RFC since I'm not sure if this upgrade behavior is more trouble than
>> > it's worth - this is a pretty invasive change. Happy to iterate further
>> > or just drop these changes.
>> > ---
>> > gcc/common/config/riscv/riscv-common.cc | 111 +++++++++++++++++++++---
>> > gcc/config/riscv/riscv-protos.h | 3 +-
>> > gcc/config/riscv/riscv-subset.h | 2 +-
>> > gcc/config/riscv/riscv-target-attr.cc | 4 +-
>> > gcc/config/riscv/riscv.cc | 7 +-
>> > gcc/config/riscv/riscv.h | 46 ++++++----
>> > 6 files changed, 137 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/gcc/common/config/riscv/riscv-common.cc
>> > b/gcc/common/config/riscv/riscv-common.cc
>> > index 1dc1d9904c7..05c26f73b73 100644
>> > --- a/gcc/common/config/riscv/riscv-common.cc
>> > +++ b/gcc/common/config/riscv/riscv-common.cc
>> > @@ -907,7 +907,7 @@ riscv_subset_list::add (const char *subset, bool
>> > implied_p)
>> > VERSION_P to determine append version info or not. */
>> >
>> > std::string
>> > -riscv_subset_list::to_string (bool version_p) const
>> > +riscv_subset_list::to_string (bool version_p, bool upgrade_exts) const
>> > {
>> > std::ostringstream oss;
>> > oss << "rv" << m_xlen;
>> > @@ -916,10 +916,17 @@ 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;
>> >
>> > +#ifndef HAVE_AS_MARCH_ZAAMO_ZALRSC
>> > + bool upgrade_zaamo_zalrsc = false;
>> > + bool has_a_ext = false;
>> > + bool insert_a_ext = false;
>> > + bool inserted_a_ext = false;
>> > + riscv_subset_t *a_subset;
>> > +#endif
>> > +
>> > /* For RISC-V ISA version 2.2 or earlier version, zicsr and zifencei is
>> > included in the base ISA. */
>> > if (riscv_isa_spec == ISA_SPEC_CLASS_2P2)
>> > @@ -945,8 +952,33 @@ riscv_subset_list::to_string (bool version_p) const
>> > 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;
>> > + /* Upgrade Zaamo/Zalrsc extensions to 'a' since binutils 2.42 and
>> > earlier
>> > + don't recognize zaamo/zalrsc. */
>> > + upgrade_zaamo_zalrsc = upgrade_exts;
>> > + if (upgrade_zaamo_zalrsc)
>> > + {
>> > + for (subset = m_head; subset != NULL; subset = subset->next)
>> > + {
>> > + if (subset->name == "a")
>> > + has_a_ext = true;
>> > + if (subset->name == "zaamo" || subset->name == "zalrsc")
>> > + insert_a_ext = true;
>> > + }
>> > + if (insert_a_ext && !has_a_ext)
>> > + {
>> > + unsigned int major_version = 0, minor_version = 0;
>> > + get_default_version ("a", &major_version, &minor_version);
>> > + a_subset = new riscv_subset_t ();
>> > + a_subset->name = "a";
>> > + a_subset->implied_p = false;
>> > + a_subset->major_version = major_version;
>> > + a_subset->minor_version = minor_version;
>> > + }
>> > + }
>> > +#else
>> > + /* Silence unused parameter warning when HAVE_AS_MARCH_ZAAMO_ZALRSC is
>> > + set. */
>> > + (void) upgrade_exts;
>> > #endif
>> >
>> > for (subset = m_head; subset != NULL; subset = subset->next)
>> > @@ -959,12 +991,29 @@ riscv_subset_list::to_string (bool version_p) const
>> > subset->name == "zicsr")
>> > continue;
>> >
>> > - if (skip_zaamo_zalrsc && subset->name == "zaamo")
>> > +#ifndef HAVE_AS_MARCH_ZAAMO_ZALRSC
>> > + if (upgrade_zaamo_zalrsc && subset->name == "zaamo")
>> > continue;
>> >
>> > - if (skip_zaamo_zalrsc && subset->name == "zalrsc")
>> > + if (upgrade_zaamo_zalrsc && subset->name == "zalrsc")
>> > continue;
>> >
>> > + if (upgrade_zaamo_zalrsc && insert_a_ext && !has_a_ext &&
>> > !inserted_a_ext)
>> > + {
>> > + gcc_assert (a_subset);
>> > + /* Insert `a` extension in cannonical order. */
>> > + if (subset_cmp (a_subset->name, subset->name) > 0)
>> > + {
>> > + oss << a_subset->name;
>> > + if (version_p)
>> > + oss << a_subset->major_version
>> > + << 'p'
>> > + << a_subset->minor_version;
>> > + inserted_a_ext = true;
>> > + }
>> > + }
>> > +#endif
>> > +
>> > /* For !version_p, we only separate extension with underline for
>> > multi-letter extension. */
>> > if (!first &&
>> > @@ -984,6 +1033,14 @@ riscv_subset_list::to_string (bool version_p) const
>> > << subset->minor_version;
>> > }
>> >
>> > +#ifndef HAVE_AS_MARCH_ZAAMO_ZALRSC
>> > + if (upgrade_zaamo_zalrsc && insert_a_ext && !has_a_ext)
>> > + {
>> > + gcc_assert (inserted_a_ext);
>> > + free (a_subset);
>> > + }
>> > +#endif
>> > +
>> > return oss.str ();
>> > }
>> >
>> > @@ -1598,10 +1655,10 @@ riscv_subset_list::finalize ()
>> > /* Return the current arch string. */
>> >
>> > std::string
>> > -riscv_arch_str (bool version_p)
>> > +riscv_arch_str (bool version_p, bool upgrade_exts)
>> > {
>> > if (current_subset_list)
>> > - return current_subset_list->to_string (version_p);
>> > + return current_subset_list->to_string (version_p, upgrade_exts);
>> > else
>> > return std::string();
>> > }
>> > @@ -1907,18 +1964,33 @@ riscv_handle_option (struct gcc_options *opts,
>> >
>> > const char *
>> > riscv_expand_arch (int argc ATTRIBUTE_UNUSED,
>> > - const char **argv)
>> > + const char **argv,
>> > + bool upgrade_exts)
>> > {
>> > gcc_assert (argc == 1);
>> > location_t loc = UNKNOWN_LOCATION;
>> > riscv_parse_arch_string (argv[0], NULL, loc);
>> > - const std::string arch = riscv_arch_str (false);
>> > + const std::string arch = riscv_arch_str (/*version_p*/ false,
>> > upgrade_exts);
>> > if (arch.length())
>> > return xasprintf ("-march=%s", arch.c_str());
>> > else
>> > return "";
>> > }
>> >
>> > +const char *
>> > +riscv_expand_arch_upgrade_exts (int argc ATTRIBUTE_UNUSED,
>> > + const char **argv)
>> > +{
>> > + return riscv_expand_arch (argc, argv, /*upgrade_exts*/ true);
>> > +}
>> > +
>> > +const char *
>> > +riscv_expand_arch_no_upgrade_exts (int argc ATTRIBUTE_UNUSED,
>> > + const char **argv)
>> > +{
>> > + return riscv_expand_arch (argc, argv, /*upgrade_exts*/ false);
>> > +}
>> > +
>> > /* Expand default -mtune option from -mcpu option, use default
>> > --with-tune value
>> > if -mcpu don't have valid value. */
>> >
>> > @@ -1938,7 +2010,8 @@ riscv_default_mtune (int argc, const char **argv)
>> >
>> > const char *
>> > riscv_expand_arch_from_cpu (int argc ATTRIBUTE_UNUSED,
>> > - const char **argv)
>> > + const char **argv,
>> > + bool upgrade_exts)
>> > {
>> > gcc_assert (argc > 0 && argc <= 2);
>> > const char *default_arch_str = NULL;
>> > @@ -1961,10 +2034,24 @@ riscv_expand_arch_from_cpu (int argc
>> > ATTRIBUTE_UNUSED,
>> > location_t loc = UNKNOWN_LOCATION;
>> >
>> > riscv_parse_arch_string (arch_str, NULL, loc);
>> > - const std::string arch = riscv_arch_str (false);
>> > + const std::string arch = riscv_arch_str (/*version_p*/ false,
>> > upgrade_exts);
>> > return xasprintf ("-march=%s", arch.c_str());
>> > }
>> >
>> > +const char *
>> > +riscv_expand_arch_from_cpu_upgrade_exts (int argc ATTRIBUTE_UNUSED,
>> > + const char **argv)
>> > +{
>> > + return riscv_expand_arch_from_cpu (argc, argv, /*upgrade_exts*/ true);
>> > +}
>> > +
>> > +const char *
>> > +riscv_expand_arch_from_cpu_no_upgrade_exts (int argc ATTRIBUTE_UNUSED,
>> > + const char **argv)
>> > +{
>> > + return riscv_expand_arch_from_cpu (argc, argv, /*upgrade_exts*/ false);
>> > +}
>> > +
>> > /* Report error if not found suitable multilib. */
>> > const char *
>> > riscv_multi_lib_check (int argc ATTRIBUTE_UNUSED,
>> > diff --git a/gcc/config/riscv/riscv-protos.h
>> > b/gcc/config/riscv/riscv-protos.h
>> > index d6473d0cd85..06b33c9f330 100644
>> > --- a/gcc/config/riscv/riscv-protos.h
>> > +++ b/gcc/config/riscv/riscv-protos.h
>> > @@ -183,7 +183,8 @@ extern tree riscv_builtin_decl (unsigned int, bool);
>> > extern void riscv_init_builtins (void);
>> >
>> > /* Routines implemented in riscv-common.cc. */
>> > -extern std::string riscv_arch_str (bool version_p = true);
>> > +extern std::string riscv_arch_str (bool version_p = true,
>> > + bool upgrade_exts = false);
>> > extern void riscv_parse_arch_string (const char *, struct gcc_options *,
>> > location_t);
>> >
>> > extern bool riscv_hard_regno_rename_ok (unsigned, unsigned);
>> > diff --git a/gcc/config/riscv/riscv-subset.h
>> > b/gcc/config/riscv/riscv-subset.h
>> > index fe7f54d8bc5..8384aab63cb 100644
>> > --- a/gcc/config/riscv/riscv-subset.h
>> > +++ b/gcc/config/riscv/riscv-subset.h
>> > @@ -90,7 +90,7 @@ public:
>> > int major_version = RISCV_DONT_CARE_VERSION,
>> > int minor_version = RISCV_DONT_CARE_VERSION)
>> > const;
>> >
>> > - std::string to_string (bool) const;
>> > + std::string to_string (bool, bool) const;
>> >
>> > unsigned xlen () const {return m_xlen;};
>> >
>> > diff --git a/gcc/config/riscv/riscv-target-attr.cc
>> > b/gcc/config/riscv/riscv-target-attr.cc
>> > index 19eb7b06d54..42c2b439e66 100644
>> > --- a/gcc/config/riscv/riscv-target-attr.cc
>> > +++ b/gcc/config/riscv/riscv-target-attr.cc
>> > @@ -367,7 +367,9 @@ riscv_process_target_attr (tree fndecl, tree args,
>> > location_t loc,
>> > /* Add the string of the target attribute to the fndecl hash table. */
>> > riscv_subset_list *subset_list = attr_parser.get_riscv_subset_list ();
>> > if (subset_list)
>> > - riscv_func_target_put (fndecl, subset_list->to_string (true));
>> > + riscv_func_target_put (fndecl,
>> > + subset_list->to_string (/*version_p*/ true,
>> > + /*upgrade_exts*/ true));
>> >
>> > return true;
>> > }
>> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> > index c17141d909a..09943389986 100644
>> > --- a/gcc/config/riscv/riscv.cc
>> > +++ b/gcc/config/riscv/riscv.cc
>> > @@ -9425,8 +9425,10 @@ riscv_sched_adjust_cost (rtx_insn *, int, rtx_insn
>> > *insn, int cost,
>> > static void
>> > riscv_emit_attribute ()
>> > {
>> > + /* Upgrade extensions if necessary for the assember to understand
>> > + Eg. Zalrsc -> a. */
>> > fprintf (asm_out_file, "\t.attribute arch, \"%s\"\n",
>> > - riscv_arch_str ().c_str ());
>> > + riscv_arch_str (/*version_p*/ true, /*upgrade_exts*/
>> > true).c_str ());
>> >
>> > fprintf (asm_out_file, "\t.attribute unaligned_access, %d\n",
>> > TARGET_STRICT_ALIGN ? 0 : 1);
>> > @@ -9468,7 +9470,8 @@ riscv_declare_function_name (FILE *stream, const
>> > char *name, tree fndecl)
>> > std::string *target_name = riscv_func_target_get (fndecl);
>> > std::string isa = target_name != NULL
>> > ? *target_name
>> > - : riscv_cmdline_subset_list ()->to_string (true);
>> > + : riscv_cmdline_subset_list ()->to_string (/*version_p*/ true,
>> > + /*upgrade_exts*/ true);
>> > fprintf (stream, "\t.option arch, %s\n", isa.c_str ());
>> > riscv_func_target_remove_and_destory (fndecl);
>> >
>> > diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
>> > index 57910eecd3e..55d0a842bf2 100644
>> > --- a/gcc/config/riscv/riscv.h
>> > +++ b/gcc/config/riscv/riscv.h
>> > @@ -46,17 +46,24 @@ along with GCC; see the file COPYING3. If not see
>> > #define RISCV_TUNE_STRING_DEFAULT "rocket"
>> > #endif
>> >
>> > -extern const char *riscv_expand_arch (int argc, const char **argv);
>> > -extern const char *riscv_expand_arch_from_cpu (int argc, const char
>> > **argv);
>> > +extern const char *riscv_expand_arch_upgrade_exts (int argc, const char
>> > **argv);
>> > +extern const char *riscv_expand_arch_no_upgrade_exts (int argc,
>> > + const char **argv);
>> > +extern const char *riscv_expand_arch_from_cpu_upgrade_exts (int argc,
>> > + const char
>> > **argv);
>> > +extern const char *riscv_expand_arch_from_cpu_no_upgrade_exts (int argc,
>> > + const char
>> > **argv);
>> > extern const char *riscv_default_mtune (int argc, const char **argv);
>> > extern const char *riscv_multi_lib_check (int argc, const char **argv);
>> > extern const char *riscv_arch_help (int argc, const char **argv);
>> >
>> > -# define EXTRA_SPEC_FUNCTIONS \
>> > - { "riscv_expand_arch", riscv_expand_arch }, \
>> > - { "riscv_expand_arch_from_cpu", riscv_expand_arch_from_cpu },
>> > \
>> > - { "riscv_default_mtune", riscv_default_mtune }, \
>> > - { "riscv_multi_lib_check", riscv_multi_lib_check }, \
>> > +# define EXTRA_SPEC_FUNCTIONS
>> > \
>> > + { "riscv_expand_arch_u", riscv_expand_arch_upgrade_exts },
>> > \
>> > + { "riscv_expand_arch_nu", riscv_expand_arch_no_upgrade_exts },
>> > \
>> > + { "riscv_expand_arch_from_cpu_nu",
>> > riscv_expand_arch_from_cpu_no_upgrade_exts }, \
>> > + { "riscv_expand_arch_from_cpu_u",
>> > riscv_expand_arch_from_cpu_upgrade_exts }, \
>> > + { "riscv_default_mtune", riscv_default_mtune },
>> > \
>> > + { "riscv_multi_lib_check", riscv_multi_lib_check },
>> > \
>> > { "riscv_arch_help", riscv_arch_help },
>> >
>> > /* Support for a compile-time default CPU, et cetera. The rules are:
>> > @@ -68,15 +75,15 @@ extern const char *riscv_arch_help (int argc, const
>> > char **argv);
>> >
>> > But using default -march/-mtune value if -mcpu don't have valid
>> > option. */
>> > #define OPTION_DEFAULT_SPECS \
>> > - {"tune", "%{!mtune=*:" \
>> > - " %{!mcpu=*:-mtune=%(VALUE)}" \
>> > - " %{mcpu=*:-mtune=%:riscv_default_mtune(%* %(VALUE))}}" }, \
>> > - {"arch", "%{!march=*:" \
>> > - " %{!mcpu=*:-march=%(VALUE)}" \
>> > - " %{mcpu=*:%:riscv_expand_arch_from_cpu(%* %(VALUE))}}" }, \
>> > - {"abi", "%{!mabi=*:-mabi=%(VALUE)}" }, \
>> > - {"isa_spec", "%{!misa-spec=*:-misa-spec=%(VALUE)}" },
>> > \
>> > - {"tls", "%{!mtls-dialect=*:-mtls-dialect=%(VALUE)}"}, \
>> > + {"tune", "%{!mtune=*:" \
>> > + " %{!mcpu=*:-mtune=%(VALUE)}" \
>> > + " %{mcpu=*:-mtune=%:riscv_default_mtune(%* %(VALUE))}}" }, \
>> > + {"arch", "%{!march=*:" \
>> > + " %{!mcpu=*:-march=%(VALUE)}" \
>> > + " %{mcpu=*:%:riscv_expand_arch_from_cpu_nu(%* %(VALUE))}}" }, \
>> > + {"abi", "%{!mabi=*:-mabi=%(VALUE)}" }, \
>> > + {"isa_spec", "%{!misa-spec=*:-misa-spec=%(VALUE)}" },
>> > \
>> > + {"tls", "%{!mtls-dialect=*:-mtls-dialect=%(VALUE)}"},
>> > \
>> >
>> > #ifdef IN_LIBGCC2
>> > #undef TARGET_64BIT
>> > @@ -103,7 +110,8 @@ extern const char *riscv_arch_help (int argc, const
>> > char **argv);
>> > #define ASM_SPEC "\
>> > %(subtarget_asm_debugging_spec) \
>> > %{" FPIE_OR_FPIC_SPEC ":-fpic} \
>> > -%{march=*} \
>> > +%{march=*:%:riscv_expand_arch_u(%*)} \
>> > +%{!march=*:%{mcpu=*:%:riscv_expand_arch_from_cpu_u(%*)}} \
>> > %{mabi=*} \
>> > %{mno-relax} \
>> > %{mbig-endian} \
>> > @@ -116,8 +124,8 @@ ASM_MISA_SPEC
>> > "%{march=help:%:riscv_arch_help()} " \
>> > "%{print-supported-extensions:%:riscv_arch_help()} " \
>> > "%{-print-supported-extensions:%:riscv_arch_help()} " \
>> > -"%{march=*:%:riscv_expand_arch(%*)} " \
>> > -"%{!march=*:%{mcpu=*:%:riscv_expand_arch_from_cpu(%*)}} "
>> > +"%{march=*:%:riscv_expand_arch_nu(%*)} " \
>> > +"%{!march=*:%{mcpu=*:%:riscv_expand_arch_from_cpu_nu(%*)}} "
>> >
>> > #define TARGET_DEFAULT_CMODEL CM_MEDLOW
>> >
>> > --
>> > 2.34.1
>> >
