[PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).

[Hafiz Abid Qadeer]

On 02/11/2021 16:27, Jakub Jelinek wrote:
> 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.
Done.

> 
>> 	(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.
Done.

> 
>> 	(gfc_match_omp_taskgroup): Use 'OMP_TASKGROUP_CLAUSES' instead of
>> 	'OMP_CLAUSE_TASK_REDUCTION'
> 
> Likewise.  Please also drop the ' characters.
Done.

> 
>> @@ -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.
Done.

> 
>> @@ -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?
Done. Also added a call to gfc_free_expr at the end to free it as n->expr points
to a copy.

> 
>> +  /* 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.

Updated patch keeps the old code but restricts it to non-composite case. For composite constructs, I
have added code at the end of gfc_split_omp_clauses to copy allocate clause to all leaf constructs
which allow it and have a privatization clause. A new testcase checks for error in this case.

Thanks
-- 
Hafiz Abid Qadeer
Mentor, a Siemens Business
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-gfortran-Add-support-for-allocate-clause-OpenMP-5.0.patch
Type: text/x-patch
Size: 68642 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20211118/63dece27/attachment-0001.bin>