[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

[Jakub Jelinek]

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

[Chung-Lin Tang]

Post by Jakub Jelinek

&& 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.

I guess Cesar was just being specific to OpenACC behavior.

Post by Jakub Jelinek

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.

Common block support and the deviceptr changes were originally separate patches.

Also, I think above changes probably could be greatly shortened if a C++ default
argument were used for gfc_match_omp_map_clause().

Post by Jakub Jelinek

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

We'll update this.

Post by Jakub Jelinek

--- 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).

This will be updated too.

Thanks,
Chung-Lin