Re: [PATCH] Support simd function declarations via a pre-include. (was: [PATCH][RFC]Overloading intrinsics)

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Nov 14, 2018 at 11:06:04AM +0100, Martin Liška wrote:
> Question I have is about default search locations for the header file. On my machine I can
> see:
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../../x86_64-pc-linux-gnu/lib/x86_64-pc-linux-gnu/9.0.0/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../../x86_64-pc-linux-gnu/lib/../lib64/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../x86_64-pc-linux-gnu/9.0.0/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../../lib64/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/lib/x86_64-pc-linux-gnu/9.0.0/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/lib/../lib64/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/usr/lib/x86_64-pc-linux-gnu/9.0.0/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/usr/lib/../lib64/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../../x86_64-pc-linux-gnu/lib/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/home/marxin/bin/gcc2/lib64/gcc/x86_64-pc-linux-gnu/9.0.0/../../../math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/lib/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> access("/usr/lib/math-vector-fortran.h", R_OK) = -1 ENOENT (No such file or directory)
> 
> Aren't these locations desired for libraries, instead of include locations?

That isn't correct indeed.
What about find_a_file (&include_prefixes, ... )?
Though, in the design where to put the file we really need to have multilib
(and multiarch on Debian/Ubuntu) in mind, because e.g. on x86_64-linux you
want to find a m64 version of the header for -m64, but a different for -m32
and there is always the possibility somebody installs a 32-bit gfortran on x86_64.

> --- a/gcc/config/gnu-user.h
> +++ b/gcc/config/gnu-user.h
> @@ -170,3 +170,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>    LD_STATIC_OPTION " --whole-archive -llsan --no-whole-archive " \
>    LD_DYNAMIC_OPTION "}}%{!static-liblsan:-llsan}"
>  #endif
> +
> +#undef TARGET_F951_NOSTDINC_OPTIONS
> +#define TARGET_F951_NOSTDINC_OPTIONS "%:fortran-header-file(-fpre-include= math-vector-fortran.h)"

Too long line, use some \s to split it up.

> +   Flags are one of:
> +     - omp-simd-notinbranch.

So omp-simd-notinbranch or omp_simd_notinbranch?
Any particular reason for this weird syntax and for not also
supporting inbranch or just simd?

> +
> +   When we come here, we have already matched the !GCC$ builtin string.  */
> +match
> +gfc_match_gcc_builtin (void)
> +{
> +  char builtin[GFC_MAX_SYMBOL_LEN + 1];
> +
> +  if (gfc_match_name (builtin) != MATCH_YES)
> +    return MATCH_ERROR;
> +
> +  gfc_gobble_whitespace ();
> +  if (gfc_match ("attributes") != MATCH_YES)
> +    return MATCH_ERROR;
> +
> +  gfc_gobble_whitespace ();
> +  if (gfc_match ("omp_simd_notinbranch") != MATCH_YES)
> +    return MATCH_ERROR;

Why so many gfc_match calls?  Can't you e.g. just do
  if (gfc_match ("%n attributes simd", builtin) != MATCH_YES)
    return MATCH_ERROR;

  int builtin_kind = 0; /* Or whatever, just want to show the parsing here.  */
  if (gfc_match ("(notinbranch)") == MATCH_YES)
    builtin_kind = -1;
  else if (gfc_match ("(inbranch)") == MATCH_YES)
    builtin_kind = 1;

The space in gfc_match indicates gfc_gobble_whitespace (), i.e. optionally
eating whitespace (note, in fixed form white space is optionally eaten
pretty much always).  If you want a mandatory space, there is "% ".
So it depends in if in fixed form we want to support e.g.
!GCC$ BUI L    TIN SINf ATTRI BUT ESSIMD(NOT IN BRANCH)
and in free form e.g.
!gcc$ builtin sinf attributessimd (notinbranch)
I wouldn't use omp_simd because in C/C++ the attribute is called simd.
> +
> +  char *r = XNEWVEC (char, strlen (builtin) + 32);
> +  sprintf (r, "__builtin_%s", builtin);
> +  vectorized_builtins.safe_push (r);

Perhaps make it vector of const char *, int pairs, so that you can
encode no argument (aka inbranch + notinbranch) vs. inbranch vs. notinbranch?

>  #define F951_OPTIONS        "%(cc1_options) %{J*} \
> -			     %{!nostdinc:-fintrinsic-modules-path finclude%s}\
> +			     %{!nostdinc:-fintrinsic-modules-path finclude%s " \
> +			       TARGET_F951_NOSTDINC_OPTIONS "}\

I wouldn't stick it inside of %{!nostdinc:, but let config/gnu-user.h decide
about that (i.e. wrap it into %{!nostdinc: ... } there).

> --- a/gcc/fortran/lang.opt
> +++ b/gcc/fortran/lang.opt
> @@ -662,6 +662,10 @@ fprotect-parens
>  Fortran Var(flag_protect_parens) Init(-1)
>  Protect parentheses in expressions.
>  
> +fpre-include=
> +Fortran RejectNegative JoinedOrMissing Var(flag_pre_include) Undocumented
> +Path to header file that should be pre-included before each compilation unit.

Why OrMissing?  If the argument is missing, that should be an error, you
can't load "".

> --- a/gcc/fortran/scanner.c
> +++ b/gcc/fortran/scanner.c
> @@ -2365,6 +2365,16 @@ load_file (const char *realfilename, const char *displayedname, bool initial)
>  	    }
>  	}
>  
> +      /* Make a guard to prevent recursive inclusion.  */
> +      static bool preinclude_done = false;
> +      if (!preinclude_done)
> +	{
> +	  preinclude_done = true;
> +	  if (flag_pre_include != NULL && !load_file (flag_pre_include, NULL,
> +						      false))
> +	    exit (FATAL_EXIT_CODE);
> +	}

Why in load_file?  I'd handle this in gfc_new_file, where it is called just
once.  Formatting - would be nicer to put && on the next line and not wrap
load_file args.

> +static const char *
> +find_fortran_header_file (int argc, const char **argv)
> +{
> +  if (argc != 2)
> +    return NULL;
> +
> +  const char *path = find_file (argv[1]);
> +  if (path != argv[1])
> +      return concat (argv[0], path, NULL);

I wouldn't call it fortran_header_file but fortran_preinclude_file
(both in what appears in spec and the name of the callback).

Plus, the patch lacks testcases.  I'd use dg-options "-nostdinc"
so that in that testcase it isn't preincluded, and have one free form and
one fixed form testcase that has various forms of the !gcc$ builtin
for a couple of builtins + some calls to them and check how they are handled
(perhaps limited to the one or two ABIs that support those)?

	Jakub