[patch] Add OpenACC Fortran support for deviceptr and variable in common blocks

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] Add OpenACC Fortran support for deviceptr and variable in common blocks

Cesar Philippidis
The attached patch adds support Fortran support for OpenACC deviceptr
and the use of common block variables in data clauses (both implicit and
explicit). This patch also relaxes the Fortran parser to not error
certain types of integral expressions and assumed-sized arrays.

With respect to those errors, I removed them because a lot of working
applications do not explicitly use type attributes (like contiguous).
Perhaps it would be better to reduce them to a warning. Any thoughts on
that? My argument for their removal is that, while the standard states
that, say, arrays must be contiguous or bad things will happen, it does
not necessary mandate that the compiler enforces it. I.e., the intent is
to set the user's expectation that things will go bad if garbage input
is fed to the accelerator. If necessary, I can push back on the OpenACC
standards committee on these issue, but don't expect a quick resolution.

In hindsight, I probably should have kept the error relaxation patches
separate. This patch includes the following patches from og8:

  * (dd8b75a) [OpenACC] Update deviceptr handling
  * (634727d) [OpenACC] Handle Fortran deviceptr clause
  * (d50862a) [Fortran] Remove pointer check in check_array_not_assumed
  * (0793cef) [OpenACC] add support for fortran common blocks
  * (bdc1acc) [Fortran] update gfortran's tile clause error handling
  * (5dc4968) Fix PR72715 "ICE in gfc_trans_omp_do, at
              fortran/trans-openmp.c:3164"

Is this patch OK for trunk? It bootstrapped / regression tested cleanly
for x86_64 with nvptx offloading.

Thanks,
Cesar

0001-deviceptr-patch-56.patch (45K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add OpenACC Fortran support for deviceptr and variable in common blocks

Jakub Jelinek
On Fri, Jun 29, 2018 at 12:04:58PM -0700, Cesar Philippidis wrote:

> 2018-06-29  Cesar Philippidis  <[hidden email]>
>    James Norris  <[hidden email]>
>
> gcc/fortran/
> * openmp.c (gfc_match_omp_map_clause): Re-write handling of the
> deviceptr clause.  Add new common_blocks argument.  Propagate it to
> gfc_match_omp_variable_list.
> (gfc_match_omp_clauses): Update calls to gfc_match_omp_map_clauses.
> (resolve_positive_int_expr): Promote the warning to an error.
> (check_array_not_assumed): Remove pointer check.
> (resolve_oacc_nested_loops): Error on do concurrent loops.
> * trans-openmp.c (gfc_omp_finish_clause): Don't create pointer data
> mappings for deviceptr clauses.
> (gfc_trans_omp_clauses): Likewise.
>
> gcc/
> * gimplify.c (enum gimplify_omp_var_data): Add GOVD_DEVICETPR.
> (oacc_default_clause): Privatize fortran common blocks.
> (omp_notice_variable): Add GOVD_DEVICEPTR attribute when appropriate.
> Defer the expansion of DECL_VALUE_EXPR for common block decls.
> (gimplify_scan_omp_clauses): Add GOVD_DEVICEPTR attribute when
> appropriate.
> (gimplify_adjust_omp_clauses_1): Set GOMP_MAP_FORCE_DEVICEPTR for
> implicit deviceptr mappings.
>
> gcc/testsuite/
> * c-c++-common/goacc/deviceptr-4.c: Update.
> * gfortran.dg/goacc/common-block-1.f90: New test.
> * gfortran.dg/goacc/common-block-2.f90: New test.
> * gfortran.dg/goacc/loop-2.f95: Update.
> * gfortran.dg/goacc/loop-3-2.f95: Update.
> * gfortran.dg/goacc/loop-3.f95: Update.
> * gfortran.dg/goacc/loop-5.f95: Update.
> * gfortran.dg/goacc/pr72715.f90: New test.
> * gfortran.dg/goacc/sie.f95: Update.
> * gfortran.dg/goacc/tile-1.f90: Update.
> * gfortran.dg/gomp/pr77516.f90: Update.
>
> libgomp/
> * oacc-parallel.c (GOACC_parallel_keyed): Handle Fortran deviceptr
> clause.
> (GOACC_data_start): Likewise.
> * testsuite/libgomp.oacc-fortran/common-block-1.f90: New test.
> * testsuite/libgomp.oacc-fortran/common-block-2.f90: New test.
> * testsuite/libgomp.oacc-fortran/common-block-3.f90: New test.
> * testsuite/libgomp.oacc-fortran/deviceptr-1.f90: New test.
> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c
> @@ -914,10 +914,11 @@ omp_inv_mask::omp_inv_mask (const omp_mask &m) : omp_mask (m)
>     mapping.  */
>  
>  static bool
> -gfc_match_omp_map_clause (gfc_omp_namelist **list, gfc_omp_map_op map_op)
> +gfc_match_omp_map_clause (gfc_omp_namelist **list, gfc_omp_map_op map_op,
> +  bool common_blocks)
>  {
>    gfc_omp_namelist **head = NULL;
> -  if (gfc_match_omp_variable_list ("", list, false, NULL, &head, true)
> +  if (gfc_match_omp_variable_list ("", list, common_blocks, NULL, &head, true)
>        == MATCH_YES)
>      {
>        gfc_omp_namelist *n;
> @@ -1039,7 +1040,7 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
>    if ((mask & OMP_CLAUSE_COPY)
>        && gfc_match ("copy ( ") == MATCH_YES
>        && gfc_match_omp_map_clause (&c->lists[OMP_LIST_MAP],
> -   OMP_MAP_TOFROM))
> +   OMP_MAP_TOFROM, openacc))

Why?  OpenMP doesn't have a copy clause, so I'd expect true here.

>      continue;
>    if (mask & OMP_CLAUSE_COPYIN)
>      {
> @@ -1047,7 +1048,7 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
>   {
>    if (gfc_match ("copyin ( ") == MATCH_YES
>        && gfc_match_omp_map_clause (&c->lists[OMP_LIST_MAP],
> -   OMP_MAP_TO))
> +   OMP_MAP_TO, true))
>      continue;

OpenMP does have a copyin clause, but it is handled below, so this one is ok.

> @@ -1156,12 +1157,12 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, const omp_mask mask,
>        && openacc
>        && gfc_match ("device ( ") == MATCH_YES
>        && gfc_match_omp_map_clause (&c->lists[OMP_LIST_MAP],
> -   OMP_MAP_FORCE_TO))
> +   OMP_MAP_FORCE_TO, false))
>      continue;
>    if ((mask & OMP_CLAUSE_DEVICEPTR)
>        && gfc_match ("deviceptr ( ") == MATCH_YES
>        && gfc_match_omp_map_clause (&c->lists[OMP_LIST_MAP],
> -   OMP_MAP_FORCE_DEVICEPTR))
> +   OMP_MAP_FORCE_DEVICEPTR, false))
>      continue;

Not sure about these, your call.  deviceptr is OpenACC specific clause,
device is in both, but means something different in OpenMP.  In any case,
haven't looked if OpenACC allows common blocks in these clauses.

> @@ -3718,8 +3719,8 @@ resolve_positive_int_expr (gfc_expr *expr, const char *clause)
>    if (expr->expr_type == EXPR_CONSTANT
>        && expr->ts.type == BT_INTEGER
>        && mpz_sgn (expr->value.integer) <= 0)
> -    gfc_warning (0, "INTEGER expression of %s clause at %L must be positive",
> - clause, &expr->where);
> +    gfc_error ("INTEGER expression of %s clause at %L must be positive",
> +       clause, &expr->where);
>  }

This affects OpenMP too and makes it inconsistent with C/C++.  Why?
If you need it for OpenACC clauses, then we need two routines.

>  static void
> @@ -3777,10 +3778,6 @@ check_array_not_assumed (gfc_symbol *sym, locus loc, const char *name)
>    if (sym->as && sym->as->type == AS_ASSUMED_RANK)
>      gfc_error ("Assumed rank array %qs in %s clause at %L",
>         sym->name, name, &loc);
> -  if (sym->as && sym->as->type == AS_DEFERRED && sym->attr.pointer
> -      && !sym->attr.contiguous)
> -    gfc_error ("Noncontiguous deferred shape array %qs in %s clause at %L",
> -       sym->name, name, &loc);
>  }

Perhaps it would help to mention oacc in the name of this routine to make
it clearer that it is OpenACC only.  The change is ok if that is what
OpenACC now allows.

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -105,6 +105,9 @@ enum gimplify_omp_var_data
>    /* Flag for GOVD_MAP: must be present already.  */
>    GOVD_MAP_FORCE_PRESENT = 524288,
>  
> +  /* Flag for OpenACC deviceptrs.  */
> +  GOVD_DEVICEPTR = (1<<21),

Please use the same style of constants as in the rest (and, you need
to adjust anyway for current trunk).

        Jakub