On 11/14/18 12:35 PM, Jakub Jelinek wrote:
> 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, ... )?
Thanks, so setting last argument to true should handle here the multilib
support.
> 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?
Questionable whether to support as current glibc vector ABI only uses
notinbranch?
>
>> +
>> + 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?
If we want to support the variants, then yes.
>
>> #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).
Sure, works for me now.
>
>> --- 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 "".
Ok.
>
>> --- 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.
Works for me.
>
>> +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).
Done.
>
> 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)?
Sure, will do as soon as final version of the patch will be done.
Thanks,
Martin
>
> Jakub
>
