Re: [PATCH v2] aarch64: Fix ACLE SME streaming mode error in neon-sve-bridge

[Richard Sandiford <richard.sandiford@arm.com>]

Richard Ball <richard.ball@arm.com> writes:
> When using LTO, handling the pragma for sme before the pragma
> for the neon-sve-bridge caused the following error on svset_neonq,
> in the neon-sve-bridge.c test.
>
> error: ACLE function '0' can only be called when SME streaming mode is enabled.
>
> This has been resolved by changing the pragma handlers to accept two modes.
> One where they add functions normally and a second in which registered_functions
> is filled with a placeholder value.
>
> By using this, the ordering of the functions can be maintained.
>
> No regressions on aarch64-none-elf.

OK, thanks.

Richard

> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-c.cc (aarch64_pragma_aarch64):
> 	Add functions_nulls parameter to pragma_handlers.
>         * config/aarch64/aarch64-protos.h: Likewise.
>         * config/aarch64/aarch64-sve-builtins.h
> 	(enum handle_pragma_index): Add enum to count
> 	number of pragmas to be handled.
>         * config/aarch64/aarch64-sve-builtins.cc
> 	(GTY): Add global variable for initial indexes
> 	and change overload_names to an array.
> 	(function_builder::function_builder):
> 	Add pragma handler information.
>         (function_builder::add_function):
> 	Add code for overwriting previous
> 	registered_functions entries.
>         (add_unique_function):
> 	Use an array to register overload_names
> 	for both pragma handler modes.
>         (add_overloaded_function): Likewise.
>         (init_builtins):
> 	Add functions_nulls parameter to pragma_handlers.
>         (handle_arm_sve_h):
> 	Initialize pragma handler information.
>         (handle_arm_neon_sve_bridge_h): Likewise.
>         (handle_arm_sme_h): Likewise.
>
> diff --git a/gcc/config/aarch64/aarch64-c.cc b/gcc/config/aarch64/aarch64-c.cc
> index b5a6917d06db41a845681ed6f13f2800c70ede10..fe1a20e4e546a68e5f7eddff3bbb0d3e831fbd9b 100644
> --- a/gcc/config/aarch64/aarch64-c.cc
> +++ b/gcc/config/aarch64/aarch64-c.cc
> @@ -344,15 +344,15 @@ aarch64_pragma_aarch64 (cpp_reader *)
>  
>    const char *name = TREE_STRING_POINTER (x);
>    if (strcmp (name, "arm_sve.h") == 0)
> -    aarch64_sve::handle_arm_sve_h ();
> +    aarch64_sve::handle_arm_sve_h (false);
>    else if (strcmp (name, "arm_sme.h") == 0)
> -    aarch64_sve::handle_arm_sme_h ();
> +    aarch64_sve::handle_arm_sme_h (false);
>    else if (strcmp (name, "arm_neon.h") == 0)
>      handle_arm_neon_h ();
>    else if (strcmp (name, "arm_acle.h") == 0)
>      handle_arm_acle_h ();
>    else if (strcmp (name, "arm_neon_sve_bridge.h") == 0)
> -    aarch64_sve::handle_arm_neon_sve_bridge_h ();
> +    aarch64_sve::handle_arm_neon_sve_bridge_h (false);
>    else
>      error ("unknown %<#pragma GCC aarch64%> option %qs", name);
>  }
> diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
> index bd719b992a545630f9ad6a73753fad55c6ac5d7e..42639e9efcf1e0f9362f759ae63a31b8eeb0d581 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -1014,9 +1014,9 @@ bool aarch64_general_check_builtin_call (location_t, vec<location_t>,
>  
>  namespace aarch64_sve {
>    void init_builtins ();
> -  void handle_arm_sve_h ();
> -  void handle_arm_sme_h ();
> -  void handle_arm_neon_sve_bridge_h ();
> +  void handle_arm_sve_h (bool);
> +  void handle_arm_sme_h (bool);
> +  void handle_arm_neon_sve_bridge_h (bool);
>    tree builtin_decl (unsigned, bool);
>    bool builtin_type_p (const_tree);
>    bool builtin_type_p (const_tree, unsigned int *, unsigned int *);
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.h b/gcc/config/aarch64/aarch64-sve-builtins.h
> index e66729ed63532811b3b16ab57ae11cb10518caca..3e0d2b8560c538d201c97fba3591ddf9893d664a 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.h
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.h
> @@ -123,6 +123,15 @@ enum units_index
>    UNITS_vectors
>  };
>  
> +/* Enumerates the pragma handlers.  */
> +enum handle_pragma_index
> +{
> +  arm_sve_handle,
> +  arm_sme_handle,
> +  arm_neon_sve_handle,
> +  NUM_PRAGMA_HANDLERS
> +};
> +
>  /* Describes the various uses of a governing predicate.  */
>  enum predication_index
>  {
> @@ -419,7 +428,7 @@ class registered_function;
>  class function_builder
>  {
>  public:
> -  function_builder ();
> +  function_builder (handle_pragma_index, bool);
>    ~function_builder ();
>  
>    void add_unique_function (const function_instance &, tree,
> @@ -453,6 +462,12 @@ private:
>  
>    /* Used for building up function names.  */
>    obstack m_string_obstack;
> +
> +  /* Used to store the index for the current function.  */
> +  unsigned int m_function_index;
> +
> +  /* Stores the mode of the current pragma handler.  */
> +  bool m_function_nulls;
>  };
>  
>  /* A base class for handling calls to built-in functions.  */
> diff --git a/gcc/config/aarch64/aarch64-sve-builtins.cc b/gcc/config/aarch64/aarch64-sve-builtins.cc
> index 11f5c5c500c8331094933cb1c1205a1360eca79b..924bfeb3e23bb1bbed79d36adf33286b87b52f85 100644
> --- a/gcc/config/aarch64/aarch64-sve-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-sve-builtins.cc
> @@ -933,14 +933,19 @@ tree acle_svprfop;
>  /* The list of all registered function decls, indexed by code.  */
>  static GTY(()) vec<registered_function *, va_gc> *registered_functions;
>  
> +/* Stores the starting function index for each pragma handler.  */
> +static unsigned int initial_indexes[NUM_PRAGMA_HANDLERS];
> +
>  /* All registered function decls, hashed on the function_instance
>     that they implement.  This is used for looking up implementations of
>     overloaded functions.  */
>  static hash_table<registered_function_hasher> *function_table;
>  
> -/* Maps all overloaded function names that we've registered so far to
> -   their associated function_instances.  The map keys are IDENTIFIER_NODEs.  */
> -static GTY(()) hash_map<tree, registered_function *> *overload_names;
> +/* Index 0 maps all overloaded function names that we've registered so far to
> +   their associated function_instances.  Index 1 does the same for functions
> +   that we've skipped over without registering.  In both cases, the map keys
> +   are IDENTIFIER_NODEs.  */
> +static GTY(()) hash_map<tree, registered_function *> *overload_names[2];
>  
>  /* True if we've already complained about attempts to use functions
>     when the required extension is disabled.  */
> @@ -1332,10 +1337,21 @@ sve_switcher::~sve_switcher ()
>    maximum_field_alignment = m_old_maximum_field_alignment;
>  }
>  
> -function_builder::function_builder ()
> +function_builder::function_builder (handle_pragma_index pragma_index,
> +				    bool function_nulls)
>  {
>    m_overload_type = build_function_type (void_type_node, void_list_node);
>    m_direct_overloads = lang_GNU_CXX ();
> +
> +  if (initial_indexes[pragma_index] == 0)
> +    {
> +      unsigned int index = vec_safe_length (registered_functions);
> +      initial_indexes[pragma_index] = index;
> +    }
> +
> +  m_function_index = initial_indexes[pragma_index];
> +  m_function_nulls = function_nulls;
> +
>    gcc_obstack_init (&m_string_obstack);
>  }
>  
> @@ -1499,9 +1515,8 @@ function_builder::add_function (const function_instance &instance,
>  				bool overloaded_p,
>  				bool placeholder_p)
>  {
> -  unsigned int code = vec_safe_length (registered_functions);
> -  code = (code << AARCH64_BUILTIN_SHIFT) | AARCH64_BUILTIN_SVE;
> -
> +  unsigned int length = vec_safe_length (registered_functions);
> +  unsigned int code = (m_function_index << AARCH64_BUILTIN_SHIFT) | AARCH64_BUILTIN_SVE;
>    /* 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.
> @@ -1515,7 +1530,7 @@ function_builder::add_function (const function_instance &instance,
>       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
> +  tree decl = placeholder_p || m_function_nulls
>      ? integer_zero_node
>      : simulate_builtin_function_decl (input_location, name, fntype,
>  				      code, NULL, attrs);
> @@ -1525,7 +1540,11 @@ function_builder::add_function (const function_instance &instance,
>    rfn.decl = decl;
>    rfn.required_extensions = required_extensions;
>    rfn.overloaded_p = overloaded_p;
> -  vec_safe_push (registered_functions, &rfn);
> +  if (m_function_index >= length)
> +    vec_safe_push (registered_functions, &rfn);
> +  else
> +    (*registered_functions)[m_function_index] = &rfn;
> +  m_function_index++;
>  
>    return rfn;
>  }
> @@ -1554,11 +1573,14 @@ add_unique_function (const function_instance &instance,
>  					   required_extensions, false, false);
>  
>    /* Enter the function into the hash table.  */
> -  hashval_t hash = instance.hash ();
> -  registered_function **rfn_slot
> -    = function_table->find_slot_with_hash (instance, hash, INSERT);
> -  gcc_assert (!*rfn_slot);
> -  *rfn_slot = &rfn;
> +  if (!m_function_nulls)
> +    {
> +      hashval_t hash = instance.hash ();
> +      registered_function **rfn_slot
> +	= function_table->find_slot_with_hash (instance, hash, INSERT);
> +      gcc_assert (!*rfn_slot);
> +      *rfn_slot = &rfn;
> +    }
>  
>    /* Also add the function under its overloaded alias, if we want
>       a separate decl for each instance of an overloaded function.  */
> @@ -1589,12 +1611,13 @@ function_builder::
>  add_overloaded_function (const function_instance &instance,
>  			 aarch64_feature_flags required_extensions)
>  {
> -  if (!overload_names)
> -    overload_names = hash_map<tree, registered_function *>::create_ggc ();
> +  auto &name_map = overload_names[m_function_nulls];
> +  if (!name_map)
> +    name_map = hash_map<tree, registered_function *>::create_ggc ();
>  
>    char *name = get_name (instance, true);
>    tree id = get_identifier (name);
> -  if (registered_function **map_value = overload_names->get (id))
> +  if (registered_function **map_value = name_map->get (id))
>      gcc_assert ((*map_value)->instance == instance
>  		&& ((*map_value)->required_extensions
>  		    & ~required_extensions) == 0);
> @@ -1603,7 +1626,7 @@ add_overloaded_function (const function_instance &instance,
>        registered_function &rfn
>  	= add_function (instance, name, m_overload_type, NULL_TREE,
>  			required_extensions, true, m_direct_overloads);
> -      overload_names->put (id, &rfn);
> +      name_map->put (id, &rfn);
>      }
>    obstack_free (&m_string_obstack, name);
>  }
> @@ -4510,9 +4533,9 @@ init_builtins ()
>    register_builtin_types ();
>    if (in_lto_p)
>      {
> -      handle_arm_sve_h ();
> -      handle_arm_sme_h ();
> -      handle_arm_neon_sve_bridge_h ();
> +      handle_arm_sve_h (false);
> +      handle_arm_sme_h (false);
> +      handle_arm_neon_sve_bridge_h (false);
>      }
>  }
>  
> @@ -4614,7 +4637,7 @@ register_svprfop ()
>  
>  /* Implement #pragma GCC aarch64 "arm_sve.h".  */
>  void
> -handle_arm_sve_h ()
> +handle_arm_sve_h (bool function_nulls_p)
>  {
>    if (function_table)
>      {
> @@ -4641,17 +4664,20 @@ handle_arm_sve_h ()
>  
>    /* Define the functions.  */
>    function_table = new hash_table<registered_function_hasher> (1023);
> -  function_builder builder;
> +  function_builder builder (arm_sve_handle, function_nulls_p);
>    for (unsigned int i = 0; i < ARRAY_SIZE (function_groups); ++i)
>      builder.register_function_group (function_groups[i]);
>  }
>  
>  /* Implement #pragma GCC aarch64 "arm_neon_sve_bridge.h".  */
>  void
> -handle_arm_neon_sve_bridge_h ()
> +handle_arm_neon_sve_bridge_h (bool function_nulls_p)
>  {
> +  if (initial_indexes[arm_sme_handle] == 0)
> +    handle_arm_sme_h (true);
> +
>    /* Define the functions.  */
> -  function_builder builder;
> +  function_builder builder (arm_neon_sve_handle, function_nulls_p);
>    for (unsigned int i = 0; i < ARRAY_SIZE (neon_sve_function_groups); ++i)
>      builder.register_function_group (neon_sve_function_groups[i]);
>  }
> @@ -4668,7 +4694,7 @@ builtin_decl (unsigned int code, bool)
>  
>  /* Implement #pragma GCC aarch64 "arm_sme.h".  */
>  void
> -handle_arm_sme_h ()
> +handle_arm_sme_h (bool function_nulls_p)
>  {
>    if (!function_table)
>      {
> @@ -4677,17 +4703,9 @@ handle_arm_sme_h ()
>        return;
>      }
>  
> -  static bool initialized_p;
> -  if (initialized_p)
> -    {
> -      error ("duplicate definition of %qs", "arm_sme.h");
> -      return;
> -    }
> -  initialized_p = true;
> -
>    sme_switcher sme;
>  
> -  function_builder builder;
> +  function_builder builder (arm_sme_handle, function_nulls_p);
>    for (unsigned int i = 0; i < ARRAY_SIZE (sme_function_groups); ++i)
>      builder.register_function_group (sme_function_groups[i]);
>  }