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

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Mon, 21 Nov 2022 20:13:40 +0100
Mikael Morin <morin-mikael@orange.fr> wrote:

> 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

> > +/* 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)

> > +	do {

> > +	} while (num_quotes % 2 && gfc_match_eos () != MATCH_YES);  
> The do-while loops are wrongly indented.
> It should be:
>    do
>      {
>        ...
>      }
>    while (...)

oops, right.

> > +	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.

yes ok, noted. It's a pity in this context, where we purely pass a blob
on to the ME but ok.

> This includes gfortran.h, so the attribute arguments need to be carried 
> around using the front-end structures (gfc_actual_arglist for example).

That's a sane rule of thumb, yes.
Usually, the parser deals with language grammar and not with pure
passthrough remarks, so that's fair. Not so much in the case of such
attribs but i see your point :)
 
> > +  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 ?

Line continuation is supported i think.
Parameter variables supposedly are or should not be supported. Why would
you do that in the context of an attribute target decl?
Either way, if the ME does not find such an fndecl, it will complain
and ignore the attribute.
I don't understand non-literal constants in this context.
This very attribute applies to decls, so the existing code supposedly
matches a comma separated list of identifiers. The usual dollar-ok
caveats apply.

As to gfc_match_actual_arglist, probably.
target_clones has
+  { "target_clones",          1, -1, true, false, false, false,
+                             dummy, NULL },
with tree-core.h struct attribute_spec, so
name, min=1, max=unbounded, decl_required=yes, ...ignore...

hence applies to functions and subroutines and the like. It does take an
unbounded list of strings, isa1, isa2, isa4, default. We could add
"default" unless seen, but i'd rather want it spelled out by the user
for the user is supposed to know what she's doing, as in c or c++.
The ME has code to sanity-check the attributes, including conflicting
(ME) attributes.

The reason why i contemplated with a separate parser was that for stuff
like regparm or sseregparm, you would want to require a single number
for the equivalent of

__attribute__((regparm(3),stdcall)

which you would provide in 2 separate !GCC$ attributes i assume.

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

Yes, many thanks for your comments.
I think there is no other non-intrusive way to pass the data through the
frontend. So for an acceptable way this means touching quite some spots
for every single ME attribute anybody would like to add in the future.
But that's how it is.