Message ID | 20210421154340.kwh6kqfsoj3v3g4k@arm.com |
---|---|
State | New |
Headers | show |
Series | [10] aarch64: Fix SVE ACLE builtins with LTO [PR99216] | expand |
Alex Coplan <alex.coplan@arm.com> writes: > Hi, > > Here is a backport of my fix for PR99216. The only change w.r.t the > original patch is a bump of lto-streamer.h:LTO_minor_version. > > Bootstrapped and regtested on aarch64-linux-gnu, no issues. > > OK for GCC 10 branch? OK, thanks. Richard > As discussed in the PR, we currently have two different numbering > schemes for SVE builtins: one for C, and one for C++. This is > problematic for LTO, where we end up getting confused about which > intrinsic we're talking about. This patch inserts placeholders into the > registered_functions vector to ensure that there is a consistent > numbering scheme for both C and C++. > > This version uses integer_zero_node as a placeholder node instead of > building a function decl. This is safe because the node is only returned > by the TARGET_BUILTIN_DECL hook, which (on AArch64) is only used for > validation when builtin decls are streamed into lto1. > > gcc/ChangeLog: > > PR target/99216 > * config/aarch64/aarch64-sve-builtins.cc > (function_builder::add_function): Add placeholder_p argument, use > placeholder decls if this is set. > (function_builder::add_unique_function): Instead of conditionally adding > direct overloads, unconditionally add either a direct overload or a > placeholder. > (function_builder::add_overloaded_function): Set placeholder_p if we're > using C++ overloads. Use the obstack for string storage instead > of relying on the tree nodes. > (function_builder::add_overloaded_functions): Don't return early for > m_direct_overloads: we need to add placeholders. > * config/aarch64/aarch64-sve-builtins.h > (function_builder::add_function): Add placeholder_p argument. > * lto-streamer.h (LTO_minor_version): Bump. > > gcc/testsuite/ChangeLog: > > PR target/99216 > * g++.target/aarch64/sve/pr99216.C: New test. > > diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc > index d534ca923d9..336a1db662b 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins.cc > +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc > @@ -995,12 +995,29 @@ registered_function & > function_builder::add_function (const function_instance &instance, > const char *name, tree fntype, tree attrs, > uint64_t required_extensions, > - bool overloaded_p) > + bool overloaded_p, > + bool placeholder_p) > { > unsigned int code = vec_safe_length (registered_functions); > code = (code << AARCH64_BUILTIN_SHIFT) | AARCH64_BUILTIN_SVE; > - tree decl = simulate_builtin_function_decl (input_location, name, fntype, > - code, NULL, attrs); > + > + /* We need to be able to generate placeholders to enusre that we have a > + consistent numbering scheme for function codes between the C and C++ > + frontends, so that everything ties up in LTO. > + > + Currently, tree-streamer-in.c:unpack_ts_function_decl_value_fields > + validates that tree nodes returned by TARGET_BUILTIN_DECL are non-NULL and > + some node other than error_mark_node. This is a holdover from when builtin > + decls were streamed by code rather than by value. > + > + Ultimately, we should be able to remove this validation of BUILT_IN_MD > + nodes and remove the target hook. For now, however, we need to appease the > + validation and return a non-NULL, non-error_mark_node node, so we > + arbitrarily choose integer_zero_node. */ > + tree decl = placeholder_p > + ? integer_zero_node > + : simulate_builtin_function_decl (input_location, name, fntype, > + code, NULL, attrs); > > registered_function &rfn = *ggc_alloc <registered_function> (); > rfn.instance = instance; > @@ -1032,7 +1049,7 @@ function_builder::add_unique_function (const function_instance &instance, > argument_types.address ()); > tree attrs = get_attributes (instance); > registered_function &rfn = add_function (instance, name, fntype, attrs, > - required_extensions, false); > + required_extensions, false, false); > > /* Enter the function into the hash table. */ > hashval_t hash = instance.hash (); > @@ -1043,16 +1060,14 @@ function_builder::add_unique_function (const function_instance &instance, > > /* Also add the function under its overloaded alias, if we want > a separate decl for each instance of an overloaded function. */ > - if (m_direct_overloads || force_direct_overloads) > + char *overload_name = get_name (instance, true); > + if (strcmp (name, overload_name) != 0) > { > - char *overload_name = get_name (instance, true); > - if (strcmp (name, overload_name) != 0) > - { > - /* Attribute lists shouldn't be shared. */ > - tree attrs = get_attributes (instance); > - add_function (instance, overload_name, fntype, attrs, > - required_extensions, false); > - } > + /* Attribute lists shouldn't be shared. */ > + tree attrs = get_attributes (instance); > + bool placeholder_p = !(m_direct_overloads || force_direct_overloads); > + add_function (instance, overload_name, fntype, attrs, > + required_extensions, false, placeholder_p); > } > > obstack_free (&m_string_obstack, name); > @@ -1073,18 +1088,19 @@ function_builder::add_overloaded_function (const function_instance &instance, > { > char *name = get_name (instance, true); > if (registered_function **map_value = m_overload_names.get (name)) > - gcc_assert ((*map_value)->instance == instance > - && ((*map_value)->required_extensions > - & ~required_extensions) == 0); > + { > + gcc_assert ((*map_value)->instance == instance > + && ((*map_value)->required_extensions > + & ~required_extensions) == 0); > + obstack_free (&m_string_obstack, name); > + } > else > { > registered_function &rfn > = add_function (instance, name, m_overload_type, NULL_TREE, > - required_extensions, true); > - const char *permanent_name = IDENTIFIER_POINTER (DECL_NAME (rfn.decl)); > - m_overload_names.put (permanent_name, &rfn); > + required_extensions, true, m_direct_overloads); > + m_overload_names.put (name, &rfn); > } > - obstack_free (&m_string_obstack, name); > } > > /* If we are using manual overload resolution, add one function decl > @@ -1094,9 +1110,6 @@ void > function_builder::add_overloaded_functions (const function_group_info &group, > mode_suffix_index mode) > { > - if (m_direct_overloads) > - return; > - > unsigned int explicit_type0 = (*group.shape)->explicit_type_suffix_p (0); > unsigned int explicit_type1 = (*group.shape)->explicit_type_suffix_p (1); > for (unsigned int pi = 0; group.preds[pi] != NUM_PREDS; ++pi) > diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h b/gcc/config/aarch64/aarch64-sve-builtins.h > index 526d9f55e7b..5ffc58cab0a 100644 > --- a/gcc/config/aarch64/aarch64-sve-builtins.h > +++ b/gcc/config/aarch64/aarch64-sve-builtins.h > @@ -337,7 +337,8 @@ private: > tree get_attributes (const function_instance &); > > registered_function &add_function (const function_instance &, > - const char *, tree, tree, uint64_t, bool); > + const char *, tree, tree, > + uint64_t, bool, bool); > > /* The function type to use for functions that are resolved by > function_resolver. */ > diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h > index 03440f0254c..7ac3b63fd33 100644 > --- a/gcc/lto-streamer.h > +++ b/gcc/lto-streamer.h > @@ -121,7 +121,7 @@ along with GCC; see the file COPYING3. If not see > form followed by the data for the string. */ > > #define LTO_major_version 9 > -#define LTO_minor_version 3 > +#define LTO_minor_version 4 > > typedef unsigned char lto_decl_flags_t; > > diff --git a/gcc/testsuite/g++.target/aarch64/sve/pr99216.C b/gcc/testsuite/g++.target/aarch64/sve/pr99216.C > new file mode 100644 > index 00000000000..61a58a7fcf4 > --- /dev/null > +++ b/gcc/testsuite/g++.target/aarch64/sve/pr99216.C > @@ -0,0 +1,5 @@ > +/* { dg-do link { target aarch64_asm_sve_ok } } */ > +/* { dg-additional-options "-flto" } */ > +#include <arm_sve.h> > +bool a; > +int main() { a = svaddv(svptrue_b8(), svdup_s8(0)); }
diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc index d534ca923d9..336a1db662b 100644 --- a/gcc/config/aarch64/aarch64-sve-builtins.cc +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc @@ -995,12 +995,29 @@ registered_function & function_builder::add_function (const function_instance &instance, const char *name, tree fntype, tree attrs, uint64_t required_extensions, - bool overloaded_p) + bool overloaded_p, + bool placeholder_p) { unsigned int code = vec_safe_length (registered_functions); code = (code << AARCH64_BUILTIN_SHIFT) | AARCH64_BUILTIN_SVE; - tree decl = simulate_builtin_function_decl (input_location, name, fntype, - code, NULL, attrs); + + /* We need to be able to generate placeholders to enusre that we have a + consistent numbering scheme for function codes between the C and C++ + frontends, so that everything ties up in LTO. + + Currently, tree-streamer-in.c:unpack_ts_function_decl_value_fields + validates that tree nodes returned by TARGET_BUILTIN_DECL are non-NULL and + some node other than error_mark_node. This is a holdover from when builtin + decls were streamed by code rather than by value. + + Ultimately, we should be able to remove this validation of BUILT_IN_MD + nodes and remove the target hook. For now, however, we need to appease the + validation and return a non-NULL, non-error_mark_node node, so we + arbitrarily choose integer_zero_node. */ + tree decl = placeholder_p + ? integer_zero_node + : simulate_builtin_function_decl (input_location, name, fntype, + code, NULL, attrs); registered_function &rfn = *ggc_alloc <registered_function> (); rfn.instance = instance; @@ -1032,7 +1049,7 @@ function_builder::add_unique_function (const function_instance &instance, argument_types.address ()); tree attrs = get_attributes (instance); registered_function &rfn = add_function (instance, name, fntype, attrs, - required_extensions, false); + required_extensions, false, false); /* Enter the function into the hash table. */ hashval_t hash = instance.hash (); @@ -1043,16 +1060,14 @@ function_builder::add_unique_function (const function_instance &instance, /* Also add the function under its overloaded alias, if we want a separate decl for each instance of an overloaded function. */ - if (m_direct_overloads || force_direct_overloads) + char *overload_name = get_name (instance, true); + if (strcmp (name, overload_name) != 0) { - char *overload_name = get_name (instance, true); - if (strcmp (name, overload_name) != 0) - { - /* Attribute lists shouldn't be shared. */ - tree attrs = get_attributes (instance); - add_function (instance, overload_name, fntype, attrs, - required_extensions, false); - } + /* Attribute lists shouldn't be shared. */ + tree attrs = get_attributes (instance); + bool placeholder_p = !(m_direct_overloads || force_direct_overloads); + add_function (instance, overload_name, fntype, attrs, + required_extensions, false, placeholder_p); } obstack_free (&m_string_obstack, name); @@ -1073,18 +1088,19 @@ function_builder::add_overloaded_function (const function_instance &instance, { char *name = get_name (instance, true); if (registered_function **map_value = m_overload_names.get (name)) - gcc_assert ((*map_value)->instance == instance - && ((*map_value)->required_extensions - & ~required_extensions) == 0); + { + gcc_assert ((*map_value)->instance == instance + && ((*map_value)->required_extensions + & ~required_extensions) == 0); + obstack_free (&m_string_obstack, name); + } else { registered_function &rfn = add_function (instance, name, m_overload_type, NULL_TREE, - required_extensions, true); - const char *permanent_name = IDENTIFIER_POINTER (DECL_NAME (rfn.decl)); - m_overload_names.put (permanent_name, &rfn); + required_extensions, true, m_direct_overloads); + m_overload_names.put (name, &rfn); } - obstack_free (&m_string_obstack, name); } /* If we are using manual overload resolution, add one function decl @@ -1094,9 +1110,6 @@ void function_builder::add_overloaded_functions (const function_group_info &group, mode_suffix_index mode) { - if (m_direct_overloads) - return; - unsigned int explicit_type0 = (*group.shape)->explicit_type_suffix_p (0); unsigned int explicit_type1 = (*group.shape)->explicit_type_suffix_p (1); for (unsigned int pi = 0; group.preds[pi] != NUM_PREDS; ++pi) diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h b/gcc/config/aarch64/aarch64-sve-builtins.h index 526d9f55e7b..5ffc58cab0a 100644 --- a/gcc/config/aarch64/aarch64-sve-builtins.h +++ b/gcc/config/aarch64/aarch64-sve-builtins.h @@ -337,7 +337,8 @@ private: tree get_attributes (const function_instance &); registered_function &add_function (const function_instance &, - const char *, tree, tree, uint64_t, bool); + const char *, tree, tree, + uint64_t, bool, bool); /* The function type to use for functions that are resolved by function_resolver. */ diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 03440f0254c..7ac3b63fd33 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -121,7 +121,7 @@ along with GCC; see the file COPYING3. If not see form followed by the data for the string. */ #define LTO_major_version 9 -#define LTO_minor_version 3 +#define LTO_minor_version 4 typedef unsigned char lto_decl_flags_t; diff --git a/gcc/testsuite/g++.target/aarch64/sve/pr99216.C b/gcc/testsuite/g++.target/aarch64/sve/pr99216.C new file mode 100644 index 00000000000..61a58a7fcf4 --- /dev/null +++ b/gcc/testsuite/g++.target/aarch64/sve/pr99216.C @@ -0,0 +1,5 @@ +/* { dg-do link { target aarch64_asm_sve_ok } } */ +/* { dg-additional-options "-flto" } */ +#include <arm_sve.h> +bool a; +int main() { a = svaddv(svptrue_b8(), svdup_s8(0)); }