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

Reply via email to