On Fri, Oct 22, 2021 at 02:05:02PM +0100, Hafiz Abid Qadeer wrote:
> This patch adds support for OpenMP 5.0 allocate clause for fortran. It does
> not
> yet support the allocator-modifier as specified in OpenMP 5.1. The allocate
> clause is already supported in C/C++.
>
> gcc/fortran/ChangeLog:
>
> * dump-parse-tree.c (show_omp_clauses): Handle OMP_LIST_ALLOCATE.
> * gfortran.h (OMP_LIST_ALLOCATE): New enum value.
> (allocate): New member in gfc_symbol.
> * openmp.c (enum omp_mask1): Add OMP_CLAUSE_ALLOCATE.
> (gfc_match_omp_clauses): Handle OMP_CLAUSE_ALLOCATE
Missing . at the end.
> (OMP_PARALLEL_CLAUSES, OMP_DO_CLAUSES, OMP_SECTIONS_CLAUSES)
> (OMP_TASK_CLAUSES, OMP_TASKLOOP_CLAUSES, OMP_TARGET_CLAUSES)
> (OMP_TEAMS_CLAUSES, OMP_DISTRIBUTE_CLAUSES)
> (OMP_SINGLE_CLAUSES): Add OMP_CLAUSE_ALLOCATE.
> (OMP_TASKGROUP_CLAUSES): New
Likewise.
> (gfc_match_omp_taskgroup): Use 'OMP_TASKGROUP_CLAUSES' instead of
> 'OMP_CLAUSE_TASK_REDUCTION'
Likewise. Please also drop the ' characters.
> @@ -1880,6 +1881,10 @@ typedef struct gfc_symbol
> according to the Fortran standard. */
> unsigned pass_as_value:1;
>
> + /* Used to check if a variable used in allocate clause has also been
> + used in privatization clause. */
> + unsigned allocate:1;
I think it would be desirable to use omp_allocate here instead
of allocate and mention OpenMP in the comment too.
Fortran has allocate statement in the language, so not pointing to
OpenMP would only cause confusion.
> @@ -1540,6 +1541,40 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const
> omp_mask mask,
> }
> continue;
> }
> + if ((mask & OMP_CLAUSE_ALLOCATE)
> + && gfc_match ("allocate ( ") == MATCH_YES)
> + {
> + gfc_expr *allocator = NULL;
> + old_loc = gfc_current_locus;
> + m = gfc_match_expr (&allocator);
> + if (m != MATCH_YES)
> + {
> + gfc_error ("Expected allocator or variable list at %C");
> + goto error;
> + }
> + if (gfc_match (" : ") != MATCH_YES)
> + {
> + /* If no ":" then there is no allocator, we backtrack
> + and read the variable list. */
> + allocator = NULL;
Isn't this a memory leak? I believe Fortran FE expressions are not GC
allocated...
So, shouldn't there be gfc_free_expr or something similar before clearing it?
> + /* Check for 2 things here.
> + 1. There is no duplication of variable in allocate clause.
> + 2. Variable in allocate clause are also present in some
> + privatization clase. */
> + for (n = omp_clauses->lists[OMP_LIST_ALLOCATE]; n; n = n->next)
> + n->sym->allocate = 0;
> +
> + gfc_omp_namelist *prev = NULL;
> + for (n = omp_clauses->lists[OMP_LIST_ALLOCATE]; n;)
> + {
> + if (n->sym->allocate == 1)
> + {
> + gfc_warning (0, "%qs appears more than once in %<allocate%> "
> + "clauses at %L" , n->sym->name, &n->where);
> + /* We have already seen this variable so it is a duplicate.
> + Remove it. */
> + if (prev != NULL && prev->next == n)
> + {
> + prev->next = n->next;
> + n->next = NULL;
> + gfc_free_omp_namelist (n, 0);
> + n = prev->next;
> + }
> +
> + continue;
> + }
> + n->sym->allocate = 1;
> + prev = n;
> + n = n->next;
> + }
> +
> + for (list = 0; list < OMP_LIST_NUM; list++)
> + switch (list)
> + {
> + case OMP_LIST_PRIVATE:
> + case OMP_LIST_FIRSTPRIVATE:
> + case OMP_LIST_LASTPRIVATE:
> + case OMP_LIST_REDUCTION:
> + case OMP_LIST_REDUCTION_INSCAN:
> + case OMP_LIST_REDUCTION_TASK:
> + case OMP_LIST_IN_REDUCTION:
> + case OMP_LIST_TASK_REDUCTION:
> + case OMP_LIST_LINEAR:
> + for (n = omp_clauses->lists[list]; n; n = n->next)
> + n->sym->allocate = 0;
> + break;
> + default:
> + break;
> + }
> +
> + for (n = omp_clauses->lists[OMP_LIST_ALLOCATE]; n; n = n->next)
> + if (n->sym->allocate == 1)
> + gfc_error ("%qs specified in 'allocate' clause at %L but not in an "
> + "explicit privatization clause", n->sym->name, &n->where);
I'm not sure this is what the standard says, certainly C/C++ FE do this
quite differently for combined/composite constructs.
In particular, we first split the clauses to the individual leaf constructs
in c_omp_split_clauses, which for allocate clause is even more complicated
because as clarified in 5.2:
"The effect of the allocate clause is as if it is applied to all leaf
constructs that permit the clause
and to which a data-sharing attribute clause that may create a private copy of
the same list item is
applied."
so there is the has_dup_allocate stuff, we first duplicate it to all leaf
constructs that allow the allocate clause and set has_dup_allocate if it is
put on more than one construct, and then if has_dup_allocate is set, do
more detailed processing. And finally then {,c_}finish_omp_clauses
diagnoses what you are trying above, but only on each leaf construct
separately.
Now, Fortran is performing the splitting of clauses only much later in
trans-openmp.c, I wonder if it doesn't have other issues on
combined/composite constructs if it performs other checks only on the
clauses on the whole combined/composite construct and not just each leaf
separately. I'd say we should move that diagnostics and perhaps other
similar later on into a separate routine that is invoked only after the
clauses are split or for non-combined/composite construct clauses.
Jakub
