Re: [PATCH 1/5] [gfortran] Add parsing support for allocate directive (OpenMP 5.0).

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Jan 13, 2022 at 02:53:16PM +0000, Hafiz Abid Qadeer wrote:
> Currently we only make use of this directive when it is associated
> with an allocate statement.

Sorry for the delay.

I'll start with a comment that allocate directive in 5.0/5.1
for Fortran is a complete mess that has been fixed only in 5.2
by splitting the directive into the allocators construct and
allocate directive.
The problem with 5.0/5.1 is that it is just ambiguous whether
!$omp allocate (list) optional-clauses
is associated with an allocate statement or not.
When it is not associated with allocate statement, it is a declarative
directive that should appear only in the specification part, when it is
associated with a allocate stmt, it should appear only in the executable
part.  And a mess starts when it is on the boundary between the two.
Now, how exactly to differentiate between the 2 I'm afraid depends
on the exact OpenMP version.
1) if we are p->state == ORDER_EXEC already, it must be associated
   with allocate-stmt (and we should error whenever it violates restrictions
   for those)
2) if (list) is missing, it must be associated with allocate-stmt
3) for 5.0 only, if allocator clause isn't specified, it must be
   not associated with allocate-stmt, but in 5.1 the clauses are optional
   also for one associated with it; if align clause is specified, it must be
   5.1
4) all the allocate directives after one that must be associated with
   allocate-stmt must be also associated with allocate-stmt
5) if variables in list are allocatable, it must be associated with
   allocate-stmt, if they aren't allocatable, it must not be associated
   with allocate-stmt

In your patch, you put ST_OMP_ALLOCATE into case_executable define,
I'm afraid due to the above we need to handle ST_OMP_ALLOCATE manually
whenever case_executable/case_omp_decl appear in parse.cc and be prepared
that it could be either declarative directive or executable construct
and resolve based on the 1-5 above into which category it belongs
(either during parsing or during resolving).  And certainly have
testsuite coverage for cases like:
  integer :: i, j
  integer, allocatable :: k(:), l(:)
!$omp allocate (i) allocator (alloc1)
!$omp allocate (j) allocator (alloc2)
!$omp allocate (k) allocator (alloc3)
!$omp allocate (l) allocator (alloc4)
  allocate (k(14), l(23))
where I think the first 2 are declarative directives and the last
2 bind to allocate-stmt (etc., cover all the cases mentioned above).

On the other side, 5.1 has:
"An allocate directive that is associated with an allocate-stmt and specifies a list must be
preceded by an executable statement or OpenMP construct."
restriction, so if we implement that, the ambiguity decreases.
We wouldn't need to worry about 3) and 5), would decide on 1) and 2) and 4)
only.

> gcc/fortran/ChangeLog:
> 
> 	* dump-parse-tree.c (show_omp_node): Handle EXEC_OMP_ALLOCATE.
> 	(show_code_node): Likewise.
> 	* gfortran.h (enum gfc_statement): Add ST_OMP_ALLOCATE.
> 	(OMP_LIST_ALLOCATOR): New enum value.
> 	(enum gfc_exec_op): Add EXEC_OMP_ALLOCATE.
> 	* match.h (gfc_match_omp_allocate): New function.
> 	* openmp.c (enum omp_mask1): Add OMP_CLAUSE_ALLOCATOR.
> 	(OMP_ALLOCATE_CLAUSES): New define.
> 	(gfc_match_omp_allocate): New function.
> 	(resolve_omp_clauses): Add ALLOCATOR in clause_names.
> 	(omp_code_to_statement): Handle EXEC_OMP_ALLOCATE.
> 	(EMPTY_VAR_LIST): New define.
> 	(check_allocate_directive_restrictions): New function.
> 	(gfc_resolve_omp_allocate): Likewise.
> 	(gfc_resolve_omp_directive): Handle EXEC_OMP_ALLOCATE.
> 	* parse.c (decode_omp_directive): Handle ST_OMP_ALLOCATE.
> 	(next_statement): Likewise.

You didn't change next_statement, but case_executable macro.
But see above.

> 	(gfc_ascii_statement): Likewise.
> 	* resolve.c (gfc_resolve_code): Handle EXEC_OMP_ALLOCATE.
> 	* st.c (gfc_free_statement): Likewise.
> 	* trans.c (trans_code): Likewise
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gfortran.dg/gomp/allocate-4.f90: New test.
> 	* gfortran.dg/gomp/allocate-5.f90: New test.
> ---

> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> @@ -921,6 +921,7 @@ enum omp_mask1
>    OMP_CLAUSE_FAIL,  /* OpenMP 5.1.  */
>    OMP_CLAUSE_WEAK,  /* OpenMP 5.1.  */
>    OMP_CLAUSE_NOWAIT,
> +  OMP_CLAUSE_ALLOCATOR,

I don't see how can you add OMP_CLAUSE_ALLOCATOR to enum omp_mask1.
OMP_MASK1_LAST is already 64, so I think the
  gcc_checking_assert (OMP_MASK1_LAST <= 64 && OMP_MASK2_LAST <= 64);
assertion would fail.
OMP_MASK2_LAST is on the other side just 30, and allocate directive
takes just allocator or in 5.1 align clauses, so both should
go to the enum omp_mask2 block.  And for newly added clauses,
we add the /* OpenMP 5.0.  */ etc. comments when the clause
appeared first (5.0 for allocator, 5.1 for align).

>    /* This must come last.  */
>    OMP_MASK1_LAST
>  };
> @@ -3568,6 +3569,7 @@ cleanup:
>  }
>  
>  
> +#define OMP_ALLOCATE_CLAUSES (omp_mask (OMP_CLAUSE_ALLOCATOR))

You define the above.

>  #define OMP_PARALLEL_CLAUSES \
>    (omp_mask (OMP_CLAUSE_PRIVATE) | OMP_CLAUSE_FIRSTPRIVATE		\
>     | OMP_CLAUSE_SHARED | OMP_CLAUSE_COPYIN | OMP_CLAUSE_REDUCTION	\
> @@ -5762,6 +5764,64 @@ gfc_match_omp_ordered_depend (void)
>    return match_omp (EXEC_OMP_ORDERED, omp_mask (OMP_CLAUSE_DEPEND));
>  }
>  
> +/* omp allocate (list) [clause-list]
> +   - clause-list:  allocator
> +*/
> +
> +match
> +gfc_match_omp_allocate (void)
> +{
> +  gfc_omp_clauses *c = gfc_get_omp_clauses ();
> +  gfc_expr *allocator = NULL;
> +  match m;
> +
> +  m = gfc_match (" (");
> +  if (m == MATCH_YES)
> +    {
> +      m = gfc_match_omp_variable_list ("", &c->lists[OMP_LIST_ALLOCATOR],
> +				       true, NULL);
> +
> +      if (m != MATCH_YES)
> +	{
> +	  /* If the list was empty, we must find closing ')'.  */

Empty list should be invalid if ( is seen, no?

> +	  m = gfc_match (")");
> +	  if (m != MATCH_YES)
> +	    return m;
> +	}
> +    }
> +
> +  if (gfc_match (" allocator ( ") == MATCH_YES)
> +    {
> +      m = gfc_match_expr (&allocator);
> +      if (m != MATCH_YES)
> +	{
> +	  gfc_error ("Expected allocator at %C");
> +	  return MATCH_ERROR;
> +	}
> +      if (gfc_match (" ) ") != MATCH_YES)
> +	{
> +	  gfc_error ("Expected ')' at %C");
> +	  gfc_free_expr (allocator);
> +	  return MATCH_ERROR;
> +	}
> +    }

But then parse the allocator clause by hand, so
OMP_ALLOCATE_CLAUSES is never used.  I think it would be better
to go through the normal clause parsing because we'll need to handle
align clause too soon and while there can be at most one allocator
clause and at most one align clause, they can appear in either order,
and there can or doesn't have to be a comma in between them.

> +      if (!omp_al || gfc_extract_int (omp_al, &tmp))
> +	  gfc_error ("%qs should use predefined allocator at %L", sym->name,
> +		     &loc);
> +    }
> +  if (ns != sym->ns)
> +    gfc_error ("%qs is not in the same scope as %<allocate%>"
> +	       " directive at %L", sym->name, &loc);
> +}
> +
> +#define EMPTY_VAR_LIST(node) \
> +  (node->ext.omp_clauses->lists[OMP_LIST_ALLOCATOR] == NULL)
> +
> +static void
> +gfc_resolve_omp_allocate (gfc_code *code, gfc_namespace *ns)
> +{
> +  gfc_alloc *al;
> +  gfc_omp_namelist *n = NULL;
> +  gfc_omp_namelist *cn = NULL;
> +  gfc_omp_namelist *p, *tail;
> +  gfc_code *cur;
> +  hash_set<gfc_symbol*> vars;
> +
> +  gfc_omp_clauses *clauses = code->ext.omp_clauses;
> +  gcc_assert (clauses);
> +  cn = clauses->lists[OMP_LIST_ALLOCATOR];
> +  gfc_expr *omp_al = cn ? cn->expr : NULL;
> +
> +  if (omp_al && (omp_al->ts.type != BT_INTEGER
> +      || omp_al->ts.kind != gfc_c_intptr_kind))

The formatting is weird, || should be below omp_al->

> +    gfc_error ("Expected integer expression of the "
> +	       "%<omp_allocator_handle_kind%> kind at %L", &omp_al->where);

	Jakub