Re: FWD: Re: OpenACC subarray specifications in the GCC Fortran front end

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi Cesar!

On Wed, 23 Jul 2014 17:42:32 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> On 07/11/2014 03:29 AM, Jakub Jelinek wrote:
> > On Fri, Jul 11, 2014 at 12:11:10PM +0200, Thomas Schwinge wrote:
> >> To avoid duplication of work: with Jakub's Fortran OpenMP 4 target
> >> changes recently committed to trunk, and now merged into gomp-4_0-branch,
> >> I have trimmed down Ilmir's patch to just the OpenACC bits, OpenMP 4
> >> target changes removed, and TODO markers added to integrate into that.
> > 
> > Resolving the TODO markers would be nice, indeed.
> 
> This patch has the openacc data clauses use the new openmp maps. In the
> process of doing so, I removed a lot of the old OMP_LIST_ enums and
> added a few OMP_MAP enums to match what the c frontend currently supports.

Thanks!

> Thomas, is this OK for gomp-4_0-branch? There are no new regressions.

A few comments.  Also copying Tobias in case he has any additional
comments on the Fortran front end changes.

OMP_LIST_DEVICEPTR remains to be converted, which can be done as a later
follow-up patch.

> 2014-07-23  Cesar Philippidis  <cesar@codesourcery.com>
> 	    Thomas Schwinge  <thomas@codesourcery.com>
> 	    Ilmir Usmanov  <i.usmanov@samsung.com>
> 
> 	gcc/fortran/
> 	* gfortran.h (gfc_omp_map_op): Add OMP_MAP_TOFROM,

OMP_MAP_TOFROM already has been present:

> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
> @@ -1111,7 +1111,13 @@ typedef enum
>    OMP_MAP_ALLOC,
>    OMP_MAP_TO,
>    OMP_MAP_FROM,
> -  OMP_MAP_TOFROM
> +  OMP_MAP_TOFROM,
> +  OMP_MAP_FORCE_ALLOC,
> +[...]

> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> @@ -448,18 +448,177 @@ match_oacc_clause_gang (gfc_omp_clauses *cp)
>  #define OMP_CLAUSE_DEVICE_RESIDENT	(1ULL << 51)
>  #define OMP_CLAUSE_HOST			(1ULL << 52)
>  #define OMP_CLAUSE_OACC_DEVICE		(1ULL << 53)
> -#define OMP_CLAUSE_OACC_COPYIN		(1ULL << 54)

> +/* Helper function for OpenACC and OpenMP clauses involving memory
> +   mapping.  */
> +
> +static bool
> +gfc_match_omp_map_clause (gfc_omp_namelist **list, gfc_omp_map_op map_op)
> +{
> +  gfc_omp_namelist **head = NULL;
> +  if (gfc_match_omp_variable_list ("", list, false, NULL, &head, true)
> +      == MATCH_YES)
> +    {
> +      gfc_omp_namelist *n;
> +      for (n = *head; n; n = n->next)
> +	n->u.map_op = map_op;
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
> +/* Match OpenACC data clauses.  */
> +
> +static bool
> +gfc_match_oacc_data_clauses (unsigned long long mask, gfc_omp_clauses *c)
> +{
> +  if ((mask & OMP_CLAUSE_COPYIN)
> +[...]
> +}
> +
> +/* Match OpenMP data clauses.  */
> +
> +static bool
> +gfc_match_omp_data_clauses (unsigned long long mask, gfc_omp_clauses *c)
> +{
> +  if ((mask & OMP_CLAUSE_COPYIN)
> +      && gfc_match_omp_variable_list ("copyin (",
> +				      &c->lists[OMP_LIST_COPYIN], true)
> +      == MATCH_YES)
> +    return true;
> +  if ((mask & OMP_CLAUSE_COPY)
> +      && gfc_match_omp_variable_list ("copy (",
> +				      &c->lists[OMP_LIST_COPY], true)
> +      == MATCH_YES)
> +    return true;

It's a bit surprising to see these two (and only these two) handled here
under the moniker OpenMP data clauses.

> +  if (mask & OMP_CLAUSE_COPYOUT)
> +    gfc_error ("Invalid OpenMP clause COPYOUT");
> +  if (mask & OMP_CLAUSE_CREATE)
> +    gfc_error ("Invalid OpenMP clause CREATE");
> +  if (mask & OMP_CLAUSE_DELETE)
> +    gfc_error ("Invalid OpenMP clause DELETE");
> +  if (mask & OMP_CLAUSE_PRESENT)
> +    gfc_error ("Invalid OpenMP clause PRESENT");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPY)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPY");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPY)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPY");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPYIN)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPYIN");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPYIN)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPYIN");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPYOUT)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPYOUT");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_COPYOUT)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_COPYOUT");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_CREATE)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_CREATE");
> +  if (mask & OMP_CLAUSE_PRESENT_OR_CREATE)
> +    gfc_error ("Invalid OpenMP clause PRESENT_OR_CREATE");

Aren't all these in fact unreachable?

> +
> +  return false;
> +}

I'd suggest to continue to handle all the data clauses...

>  
>  /* Match OpenMP and OpenACC directive clauses. MASK is a bitmask of
>     clauses that are allowed for a particular directive.  */
>  
>  static match
>  gfc_match_omp_clauses (gfc_omp_clauses **cp, unsigned long long mask,
> -		       bool first = true, bool needs_space = true)
> +		       bool first = true, bool needs_space = true,
> +		       bool openacc = false)
>  {
>    gfc_omp_clauses *c = gfc_get_omp_clauses ();
>    locus old_loc;
> @@ -533,181 +692,109 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, unsigned long long mask,
>        if ((mask & OMP_CLAUSE_NUM_THREADS) && c->num_threads == NULL
>  	  && gfc_match ("num_threads ( %e )", &c->num_threads) == MATCH_YES)
>  	continue;
> +      if ((mask & OMP_CLAUSE_NUM_GANGS) && c->num_gangs_expr == NULL
> +	  && gfc_match ("num_gangs ( %e )", &c->num_gangs_expr) == MATCH_YES)
> +	continue;
> +      if ((mask & OMP_CLAUSE_NUM_WORKERS) && c->num_workers_expr == NULL
> +	  && gfc_match ("num_workers ( %e )", &c->num_workers_expr)
> +	  == MATCH_YES)
> +	continue;
> +      if ((mask & OMP_CLAUSE_TILE)
> +	  && match_oacc_expr_list ("tile (", &c->tile_list, true) == MATCH_YES)
> +	continue;
> +      if ((mask & OMP_CLAUSE_SEQ) && !c->seq
> +	  && gfc_match ("seq") == MATCH_YES)
> +	{
> +	  c->seq = true;
> +	  needs_space = true;
> +	  continue;
> +	}
> +      if ((mask & OMP_CLAUSE_INDEPENDENT) && !c->independent
> +	  && gfc_match ("independent") == MATCH_YES)
> +	{
> +	  c->independent = true;
> +	  needs_space = true;
> +	  continue;
> +	}
> +      if ((mask & OMP_CLAUSE_AUTO) && !c->par_auto
> +	        && gfc_match ("auto") == MATCH_YES)
> +	{
> +	  c->par_auto = true;
> +	  needs_space = true;
> +	  continue;
> +	}
> +      if ((mask & OMP_CLAUSE_WAIT) && !c->wait
> +	        && gfc_match ("wait") == MATCH_YES)
> +	{
> +	  c->wait = true;
> +	  match_oacc_expr_list (" (", &c->wait_list, false);
> +	  continue;
> +	}
> +      /* Common, in the sense that no special handling is required,
> +	 OpenACC and OpenMP data clauses.  */
>        if ((mask & OMP_CLAUSE_PRIVATE)
>  	  && gfc_match_omp_variable_list ("private (",
>  					  &c->lists[OMP_LIST_PRIVATE], true)
> -	     == MATCH_YES)
> +	  == MATCH_YES)
>  	continue;
>        if ((mask & OMP_CLAUSE_FIRSTPRIVATE)
>  	  && gfc_match_omp_variable_list ("firstprivate (",
>  					  &c->lists[OMP_LIST_FIRSTPRIVATE],
>  					  true)
> -	     == MATCH_YES)
> +	  == MATCH_YES)
>  	continue;
>        if ((mask & OMP_CLAUSE_LASTPRIVATE)
>  	  && gfc_match_omp_variable_list ("lastprivate (",
>  					  &c->lists[OMP_LIST_LASTPRIVATE],
>  					  true)
> -	     == MATCH_YES)
> +	  == MATCH_YES)
>  	continue;
>        if ((mask & OMP_CLAUSE_COPYPRIVATE)
>  	  && gfc_match_omp_variable_list ("copyprivate (",
>  					  &c->lists[OMP_LIST_COPYPRIVATE],
>  					  true)
> -	     == MATCH_YES)
> +	  == MATCH_YES)
>  	continue;
>        if ((mask & OMP_CLAUSE_SHARED)
>  	  && gfc_match_omp_variable_list ("shared (",
>  					  &c->lists[OMP_LIST_SHARED], true)
> -	     == MATCH_YES)
> -	continue;
> -      if ((mask & OMP_CLAUSE_COPYIN)
> -	  && gfc_match_omp_variable_list ("copyin (",
> -					  &c->lists[OMP_LIST_COPYIN], true)
> -	     == MATCH_YES)
> -	continue;
> -      if ((mask & OMP_CLAUSE_NUM_GANGS) && c->num_gangs_expr == NULL
> -	  && gfc_match ("num_gangs ( %e )", &c->num_gangs_expr) == MATCH_YES)
> -	continue;
> -      if ((mask & OMP_CLAUSE_NUM_WORKERS) && c->num_workers_expr == NULL
> -	  && gfc_match ("num_workers ( %e )", &c->num_workers_expr)
>  	  == MATCH_YES)
>  	continue;
> -      if ((mask & OMP_CLAUSE_COPY)
> -	  && gfc_match_omp_variable_list ("copy (",
> -					  &c->lists[OMP_LIST_COPY], true)
> -	     == MATCH_YES)
> -	continue;
> -      if ((mask & OMP_CLAUSE_OACC_COPYIN)
> -	  && gfc_match_omp_variable_list ("copyin (",
> -					  &c->lists[OMP_LIST_OACC_COPYIN], true)
> -	     == MATCH_YES)
> -	continue;
> -      if ((mask & OMP_CLAUSE_COPYOUT)
> -	  && gfc_match_omp_variable_list ("copyout (",
> -					  &c->lists[OMP_LIST_COPYOUT], true)
> -	     == MATCH_YES)
> -	continue;
> -[...]

... in here, and either guard them by Âif (openacc)Â as apppropriate, or
continue using the OMP_CLAUSE_OACC_COPYIN (which you axed).  (I
understand that one to be the only conflicting one?)

>  static void
>  resolve_omp_clauses (gfc_code *code, locus *where,
> -		     gfc_omp_clauses *omp_clauses, gfc_namespace *ns)
> +		     gfc_omp_clauses *omp_clauses, gfc_namespace *ns,
> +		     bool openacc = false)
>  {
>    gfc_omp_namelist *n;
>    gfc_expr_list *el;
> @@ -2794,7 +2893,7 @@ resolve_omp_clauses (gfc_code *code, locus *where,
>  	&& list != OMP_LIST_LASTPRIVATE
>  	&& list != OMP_LIST_ALIGNED
>  	&& list != OMP_LIST_DEPEND
> -	&& list != OMP_LIST_MAP
> +	&& (list != OMP_LIST_MAP || openacc)
>  	&& list != OMP_LIST_FROM
>  	&& list != OMP_LIST_TO)
>        for (n = omp_clauses->lists[list]; n; n = n->next)
> @@ -2941,53 +3040,59 @@ resolve_omp_clauses (gfc_code *code, locus *where,
>  	  case OMP_LIST_TO:
>  	  case OMP_LIST_FROM:
>  	    for (; n != NULL; n = n->next)
> +	      {
>  [...]
> +		else if (openacc)
> +		  resolve_oacc_data_clauses (n->sym, *where,
> +					     clause_names[list]);
> +	      }

Is that special case only for deviceptr?


GrÃÃe,
 Thomas

Attachment: pgplzYSkZLTfn.pgp
Description: PGP signature