On Wed, Oct 06, 2021 at 12:39:01PM +0100, Kwok Cheung Yeung wrote:
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -11599,8 +11599,11 @@ omp_construct_selector_matches (enum tree_code
> *constructs, int nconstructs,
> }
> }
> if (!target_seen
> - && lookup_attribute ("omp declare target block",
> - DECL_ATTRIBUTES (current_function_decl)))
> + && (lookup_attribute ("omp declare target block",
> + DECL_ATTRIBUTES (current_function_decl))
> + || (lang_GNU_Fortran ()
> + && lookup_attribute ("omp declare target",
> + DECL_ATTRIBUTES (current_function_decl)))))
> {
> if (scores)
> codes.safe_push (OMP_TARGET);
So, as I wrote in the other mail, my preference would be to drop this hunk
and adjust testcases appropriately (perhaps with a comment).
The 5.1 way will then be different for all 3 languages and 5.2 way as well.
> + const char *str = TREE_STRING_POINTER (TREE_VALUE (t2));
> + if (!strcmp (str, props[i].props[j])
> + && ((size_t) TREE_STRING_LENGTH (TREE_VALUE (t2))
> + == strlen (str) + (lang_GNU_Fortran () ? 0 : 1)))
This is a little bit strange but if all identifiers from Fortran FE behave
that way and differently from C/C++ FEs, I guess we can live with that
(multiple occurrences thereof).
> + DECL_ATTRIBUTES (base_fn_decl)
> + = tree_cons (
> + get_identifier ("omp declare variant base"),
> + build_tree_list (gfc_get_symbol_decl (variant_proc_sym),
> + set_selectors),
> + DECL_ATTRIBUTES (base_fn_decl));
Perhaps that is just my private coding convention preference, but I really
don't like these calls with function (
on one line and less indented arguments on another one. I'd find it more
readable to do use temporaries where possible,
tree id = get_identifier ("omp declare variant base");
tree var = gfc_get_symbol_decl (variant_proc_sym);
DECL_ATTRIBUTES (base_fn_decl)
= tree_cons (id, build_tree_list (var, set_selectors),
DECL_ATTRIBUTES (base_fn_decl));
is IMHO more radable and fits on fewer lines even.
Other than that the patch looks mostly good, what I miss in the testcases
though is Fortran specific stuff, e.g. I couldn't find a single testcase
that uses the
!$omp declare variant (procname:variantprocname) match (construct={parallel})
syntax and couldn't find testcase coverage or resolving of the Fortran
specific declare variant restrictions.
See OpenMP 5.0 [60:1-12], which includes
base-proc-name must not be a generic name, procedure pointer, or entry name.
and
If base-proc-name is omitted then the declare variant directive must appear in
the
specification part of a subroutine subprogram or a function subprogram.
etc. So unless I've completely missed that in the patch somewhere, please
try to add new testcases (i.e. with no c-c++-common counterparts) that test
all those restrictions in there, have one !$omp declare variant with
base-proc-name that is a generic name and dg-error for it, another one
for procedure pointer, another one for entry name, another one for
!$omp declare variant with base-proc-name omitted that appears where it
isn't allowed, some !$omp declare variant (both with and without
proc-base-name) that appears e.g. in execution part, etc.
My Fortran knowledge is rusty, but I hope Tobias could help there if needed,
and if some of the restrictions make no sense, look at what has changed in
5.1 or 5.2 current state if it hasn't been clarified.
Jakub
