[PATCH] Support simd function declarations via a pre-include.

[Martin Liška]

On 11/16/18 2:49 PM, Jakub Jelinek wrote:
> On Fri, Nov 16, 2018 at 02:24:42PM +0100, Martin Liška wrote:
>> +  if (gfc_match (" (%n) attributes simd", builtin) != MATCH_YES)
>> +    return MATCH_ERROR;
>> +
>> +  int builtin_kind = 0;
>> +  if (gfc_match (" (notinbranch)") == MATCH_YES)
> 
> I think you need " ( notinbranch )" here.
> 
>> +    builtin_kind = -1;
>> +  else if (gfc_match (" (inbranch)") == MATCH_YES)
>> +    builtin_kind = 1;
> 
> And similarly here (+ testsuite coverage for whether you can in free form
> insert spaces in all the spots that should be allowed).
> !gcc$ builtin ( sinf ) attributes simd ( notinbranch )  ! comment
> e.g. should be valid in free form (and fixed form too).
> 
>> --- a/gcc/fortran/gfortran.h
>> +++ b/gcc/fortran/gfortran.h
>> @@ -2764,6 +2764,18 @@ bool gfc_in_match_data (void);
>>  match gfc_match_char_spec (gfc_typespec *);
>>  extern int directive_unroll;
>>  
>> +/* Tuple for parsing of vectorized built-ins.  */
>> +struct vect_builtin_tuple
>> +{
>> +  vect_builtin_tuple (const char *n, int t): name (n), simd_type (t)
> 
> gfc_vect_builtin_tuple ?
> + document what the simd_type is (or make it enum or whatever).
> One option would be enum omp_clause_code and use OMP_CLAUSE_ERROR for
> the case where the argument isn't specified, but I think generally
> gfortran.h doesn't depend on tree* stuff and wants to have its own
> enums etc.
> 
>> +extern vec<vect_builtin_tuple> vectorized_builtins;
> 
> gfc_vectorized_builtins ?
> 
>> --- a/gcc/fortran/trans-intrinsic.c
>> +++ b/gcc/fortran/trans-intrinsic.c
>> @@ -597,7 +597,61 @@ define_quad_builtin (const char *name, tree type, bool is_const)
>>    return fndecl;
>>  }
>>  
>> +/* Add SIMD attribute for FNDECL built-in if the built-in
>> +   name is in VECTORIZED_BUILTINS.  */
>> +#include "print-tree.h"
> 
> If you need to include a header, include it at the start of the file.
> 
>> +static void
>> +add_simd_flag_for_built_in (tree fndecl)
>> +{
>> +  if (fndecl == NULL_TREE)
>> +    return;
>> +
>> +  const char *name = IDENTIFIER_POINTER (DECL_NAME (fndecl));
>> +  for (unsigned i = 0; i < vectorized_builtins.length (); i++)
>> +    if (strcmp (vectorized_builtins[i].name, name) == 0)
> 
> How many add_simd_flag_for_built_in calls are we expecting and how many
> vectorized_builtins.length ()?  If it is too much, perhaps e.g. sort
> the vector by name and do a binary search.  At least if it turns out to be
> non-trivial compile time.
>> +
>> +  vectorized_builtins.truncate (0);
> 
> That is a memory leak, right?  The names are malloced.
> And why truncate rather than release?
>> +  const char *path = find_a_file (&include_prefixes, argv[1], R_OK, true);
>> +  if (path != NULL)
>> +      return concat (argv[0], path, NULL);
> 
> Formatting.
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/simd-builtins-1.h
>> @@ -0,0 +1,4 @@
>> +!GCC$ builtin (sinf) attributes simd
>> +!GCC$ builtin (sinf) attributes simd (inbranch)
>> +!GCC$ builtin (sinf) attributes simd (notinbranch)
>> +!GCC$ builtin (cosf) attributes simd (notinbranch)
> 
> Are you sure it is a good idea to have the 3 first lines for the same
> builtin, rather than different?
> 
> It should be testsuite covered what we do in that case, but with the above
> you don't cover what happens e.g. with notinbranch alone, or no argument.
> 
> Plus, as I said, I think you should have one *.f and one *.f90 test where
> you just use many of those !gcc$ builtin lines with spaces in various spots
> to verify it is parsed properly.
> 
> 	Jakub
> 

Hi.

I'm sending version, I changed the container to hash_map that should provide
faster look up.

I've been testing the patch right now.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Support-simd-function-declarations-via-a-pre-include.patch
Type: text/x-patch
Size: 17693 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20181116/b92da0d8/attachment.bin>