Re: [PATCH 2/2] Fortran: add attribute target_clones

[Mikael Morin <morin-mikael@orange.fr>]

Hello,

Le 09/11/2022 à 20:02, Bernhard Reutner-Fischer via Fortran a écrit :
> Hi!
> 
> Add support for attribute target_clones:
> !GCC$ ATTRIBUTES target_clones("arch1", "arch3","default") :: mysubroutine
> 
> Bootstrapped and regtested on x86_64-unknown-linux with
> --target_board=unix'{-m32,-m64}'.
> OK for trunk?
> 
> gcc/fortran/ChangeLog:
> 
> 	* decl.cc: Include fold-const.h for size_int.
> 	(gfc_match_gcc_attribute_args): New internal helper function.
> 	(gfc_match_gcc_attributes): Handle target_clones.
> 	* f95-lang.cc (struct attribute_spec): Add target and
> 	target_clones entries.
> 	* gfortran.h (ext_attr_id_t): Add EXT_ATTR_TARGET_CLONES.
> 	(struct symbol_attribute): Add field ext_attr_args.
> 	* trans-decl.cc (add_attributes_to_decl): Also add ext_attr_args
> 	to the decl's attributes.
> 	* gfortran.texi: Document attribute target_clones.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gfortran.dg/attr_target_clones-1.F90: New test.
> 
> Cc: gfortran ML <fortran@gcc.gnu.org>
> ---
>   gcc/fortran/decl.cc                           | 104 ++++++++++++++++++
>   gcc/fortran/f95-lang.cc                       |   4 +
>   gcc/fortran/gfortran.h                        |   2 +
>   gcc/fortran/gfortran.texi                     |  31 ++++++
>   gcc/fortran/trans-decl.cc                     |   3 +
>   .../gfortran.dg/attr_target_clones-1.F90      |  30 +++++
>   6 files changed, 174 insertions(+)
>   create mode 100644 gcc/testsuite/gfortran.dg/attr_target_clones-1.F90
> 
> diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
> index 0f9b2ced4c2..3a619dbdd34 100644
> --- a/gcc/fortran/decl.cc
> +++ b/gcc/fortran/decl.cc
(...)
> @@ -11709,6 +11710,96 @@ gfc_match_final_decl (void)
>     return MATCH_YES;
>   }
>   
> +/* Internal helper to parse attribute argument list.
> +   If REQUIRE_STRING is true, then require a string.
> +   If ALLOW_MULTIPLE is true, allow more than one arg.
> +   If multiple arguments are passed, require braces around them.
> +   Returns a tree_list of arguments or NULL_TREE.  */
> +static tree
> +gfc_match_gcc_attribute_args (bool require_string, bool allow_multiple)
> +{
> +  tree attr_args = NULL_TREE, attr_arg;
> +  char name[GFC_MAX_SYMBOL_LEN + 1];
> +  unsigned pos = 0;
> +  gfc_char_t c;
> +
> +  /* When we get here, we already parsed
> +     !GCC$ ATTRIBUTES ATTRIBUTE_NAME
> +     Now parse the arguments. These could be one of
> +       "single_string_literal"
> +       ( "str_literal_1" , "str_literal_2" )
> +   */
> +
> +  gfc_gobble_whitespace ();
> +
> +  if (allow_multiple && gfc_match_char ('(') != MATCH_YES)
> +    {
> +      gfc_error ("expected '(' at %C");
> +      return NULL_TREE;
> +    }
> +
> +  if (require_string)
> +    {
> +      do {
> +	if (pos)
> +	  {
> +	    if (!allow_multiple)
> +	      {
> +		gfc_error ("surplus argument at %C");
> +		return NULL_TREE;
> +	      }
> +	    gfc_next_ascii_char (); /* Consume the comma.  */
> +	  }
> +	pos = 0;
> +	gfc_gobble_whitespace ();
> +	unsigned char num_quotes = 0;
> +	do {
> +	  c = gfc_next_char_literal (NONSTRING);
> +	  if (c == '"')
> +	    {
> +	      num_quotes++;
> +	      continue; /* Skip the quote */
> +	    }
> +	  name[pos++] = c;
> +	  if (pos >= GFC_MAX_SYMBOL_LEN)
> +	    {
> +	      gfc_error ("attribute argument truncated at %C");
> +	      return NULL_TREE;
> +	    }
> +	} while (num_quotes % 2 && gfc_match_eos () != MATCH_YES);
The do-while loops are wrongly indented.
It should be:
   do
     {
       ...
     }
   while (...)

> +	if (pos < 1)
> +	  {
> +	    gfc_error ("expected argument at %C");
> +	    return NULL_TREE;
> +	  }
> +	if (num_quotes != 2)
> +	  {
> +	    gfc_error ("invalid string literal at %C");
> +	    return NULL_TREE;
> +	  }
> +	name[pos] = '\0'; /* Redundant wrt build_string.  */
> +	tree str = build_string (pos, name);
> +	/* Compare with c-family/c-common.cc: fix_string_type.  */
> +	tree i_type = build_index_type (size_int (pos));
> +	tree a_type = build_array_type (char_type_node, i_type);
> +	TREE_TYPE (str) = a_type;
> +	TREE_READONLY (str) = 1;
> +	TREE_STATIC (str) = 1;
> +	attr_arg = build_tree_list (NULL_TREE, str);
> +	attr_args = chainon (attr_args, attr_arg);
Same comment as for the flatten attribute:
please no tree stuff out of the trans-*.cc files.
This includes gfortran.h, so the attribute arguments need to be carried 
around using the front-end structures (gfc_actual_arglist for example).

> +
> +	gfc_gobble_whitespace ();
> +      } while (gfc_peek_ascii_char () == ',');
> +    }
> +
> +  if (allow_multiple && gfc_match_char (')') != MATCH_YES)
> +    {
> +      gfc_error ("expected ')' at %C");
> +      return NULL_TREE;
> +    }
> +
> +  return attr_args;
> +}
I'm not sure this function need to do all the parsing manually.
I would rather use gfc_match_actual_arglist, or maybe implement the 
function as a wrapper around it.
What is allowed here?  Are non-literal constants allowed, for example 
parameter variables?  Is line continuation supported ?

Nothing (bad) to say about the rest, but there is enough to change with 
the above comments.

Mikael