Hi Cesar! On Wed, 23 Jul 2014 17:42:32 -0700, Cesar Philippidis <ce...@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 <ce...@codesourcery.com>
> Thomas Schwinge <tho...@codesourcery.com>
> Ilmir Usmanov <i.usma...@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
pgp7Ax05s0g1h.pgp
Description: PGP signature
