Re: [OpenACC 0/7] host_data construct

[Thomas Schwinge]

Hi!

On Wed, 2 Dec 2015 16:58:45 +0100, I wrote:
> On Mon, 30 Nov 2015 19:30:34 +, Julian Brown  
> wrote:
> > --- a/libgomp/oacc-parallel.c
> > +++ b/libgomp/oacc-parallel.c
> 
> > +void
> > +GOACC_host_data (int device, size_t mapnum,
> > +void **hostaddrs, size_t *sizes, unsigned short *kinds)
> > +{
> > +[...]
> > +}
> 
> Isn't that identical to GOACC_data_start?  Can we thus get rid of it?

Yes, we can.  As GOACC_host_data has not been part of GCC 5's libgomp
ABI, it's OK to just remove it; committed "as obvious" in r233074:

commit 2bf3f448431be10baa9755df5faeed6b2f6508f8
Author: tschwinge 
Date:   Tue Feb 2 13:53:55 2016 +

Merge BUILT_IN_GOACC_HOST_DATA into BUILT_IN_GOACC_DATA_START

gcc/
* omp-builtins.def (BUILT_IN_GOACC_HOST_DATA): Remove.
* omp-low.c (expand_omp_target): Use BUILT_IN_GOACC_DATA_START
instead.
libgomp/
* libgomp.map (GOACC_2.0): Remove GOACC_host_data.
* oacc-parallel.c (GOACC_host_data): Remove function definition.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@233074 
138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog   |  6 ++
 gcc/omp-builtins.def|  2 --
 gcc/omp-low.c   |  5 +
 libgomp/ChangeLog   |  3 +++
 libgomp/libgomp.map |  1 -
 libgomp/oacc-parallel.c | 40 
 6 files changed, 10 insertions(+), 47 deletions(-)

diff --git gcc/ChangeLog gcc/ChangeLog
index 05741331..9a2cec8 100644
--- gcc/ChangeLog
+++ gcc/ChangeLog
@@ -1,3 +1,9 @@
+2016-02-02  Thomas Schwinge  
+
+   * omp-builtins.def (BUILT_IN_GOACC_HOST_DATA): Remove.
+   * omp-low.c (expand_omp_target): Use BUILT_IN_GOACC_DATA_START
+   instead.
+
 2016-02-02  Richard Biener  
 
PR tree-optimization/69606
diff --git gcc/omp-builtins.def gcc/omp-builtins.def
index 60199b0..ea012df 100644
--- gcc/omp-builtins.def
+++ gcc/omp-builtins.def
@@ -47,8 +47,6 @@ DEF_GOACC_BUILTIN (BUILT_IN_GOACC_UPDATE, "GOACC_update",
 DEF_GOACC_BUILTIN (BUILT_IN_GOACC_WAIT, "GOACC_wait",
   BT_FN_VOID_INT_INT_VAR,
   ATTR_NOTHROW_LIST)
-DEF_GOACC_BUILTIN (BUILT_IN_GOACC_HOST_DATA, "GOACC_host_data",
-  BT_FN_VOID_INT_SIZE_PTR_PTR_PTR, ATTR_NOTHROW_LIST)
 
 DEF_GOACC_BUILTIN_COMPILER (BUILT_IN_ACC_ON_DEVICE, "acc_on_device",
BT_FN_INT_INT, ATTR_CONST_NOTHROW_LEAF_LIST)
diff --git gcc/omp-low.c gcc/omp-low.c
index 0b70274..d41688b 100644
--- gcc/omp-low.c
+++ gcc/omp-low.c
@@ -13186,6 +13186,7 @@ expand_omp_target (struct omp_region *region)
   start_ix = BUILT_IN_GOACC_PARALLEL;
   break;
 case GF_OMP_TARGET_KIND_OACC_DATA:
+case GF_OMP_TARGET_KIND_OACC_HOST_DATA:
   start_ix = BUILT_IN_GOACC_DATA_START;
   break;
 case GF_OMP_TARGET_KIND_OACC_UPDATE:
@@ -13197,9 +13198,6 @@ expand_omp_target (struct omp_region *region)
 case GF_OMP_TARGET_KIND_OACC_DECLARE:
   start_ix = BUILT_IN_GOACC_DECLARE;
   break;
-case GF_OMP_TARGET_KIND_OACC_HOST_DATA:
-  start_ix = BUILT_IN_GOACC_HOST_DATA;
-  break;
 default:
   gcc_unreachable ();
 }
@@ -13324,7 +13322,6 @@ expand_omp_target (struct omp_region *region)
 case BUILT_IN_GOACC_DATA_START:
 case BUILT_IN_GOACC_DECLARE:
 case BUILT_IN_GOMP_TARGET_DATA:
-case BUILT_IN_GOACC_HOST_DATA:
   break;
 case BUILT_IN_GOMP_TARGET:
 case BUILT_IN_GOMP_TARGET_UPDATE:
diff --git libgomp/ChangeLog libgomp/ChangeLog
index 6c9bf6a..250240d 100644
--- libgomp/ChangeLog
+++ libgomp/ChangeLog
@@ -1,5 +1,8 @@
 2016-02-02  Thomas Schwinge  
 
+   * libgomp.map (GOACC_2.0): Remove GOACC_host_data.
+   * oacc-parallel.c (GOACC_host_data): Remove function definition.
+
* testsuite/lib/libgomp.exp: Skip hsa offloading for OpenACC test
cases.
 
diff --git libgomp/libgomp.map libgomp/libgomp.map
index ea9344d..4d42c42 100644
--- libgomp/libgomp.map
+++ libgomp/libgomp.map
@@ -394,7 +394,6 @@ GOACC_2.0.1 {
   global:
GOACC_declare;
GOACC_parallel_keyed;
-   GOACC_host_data;
 } GOACC_2.0;
 
 GOMP_PLUGIN_1.0 {
diff --git libgomp/oacc-parallel.c libgomp/oacc-parallel.c
index f22ba41..bc24651 100644
--- libgomp/oacc-parallel.c
+++ libgomp/oacc-parallel.c
@@ -490,46 +490,6 @@ GOACC_wait (int async, int num_waits, ...)
 goacc_thread ()->dev->openacc.async_wait_all_async_func (acc_async_noval);
 }
 
-void
-GOACC_host_data (int device, size_t mapnum,
-void **hostaddrs, size_t *sizes, unsigned short *kinds)
-{
-  bool host_fallback = device == GOMP_DEVICE_HOST_FALLBACK;
-  struct target_mem_desc *tgt;
-
-#ifdef HAVE_INTTYPES_H
-  gomp_debug (0, "%s: mapnum=%"PRIu64", hostaddrs=%p, size=%p, kinds=%p\n",
- __FUNCTION__, (uint64_t) mapnum, hostaddrs, sizes, kinds);
-#else
-  gomp_debug (0, "%s: mapnum=%lu, hostaddrs=%p, sizes=%p, kinds=%p\n",
- __FUNCTION__, (unsigned long) mapnum, hostaddrs, si

Re: [OpenACC 0/7] host_data construct

[Julian Brown]

On Thu, 19 Nov 2015 16:57:23 +0100
Jakub Jelinek  wrote:

> If it is unclear, I think disallowing acc {parallel,kernels} inside of
> acc host_data might be too big hammer, but perhaps just erroring out
> or warning during gimplification that if you (explicitly or
> implicitly) try to map a var that is in use_device clause in some
> outer context, it is either wrong, unsupported or will not do what
> users think?

I think we can only assume that trying to map a variable declared in
a surrounding use_device clause is undefined behaviour. I haven't had
any response to my questions about host_data & deviceptr on the OpenACC
list.

> > #pragma acc host_data use_device(x)
> > {
> >   target_primitive(x);
> >   #pragma acc parallel deviceptr(x)
> >   {
> > ...
> >   }
> > }
> 
> Is deviceptr as above meant to work?  That is the OpenACC counterpart
> of is_device_ptr, right?  If yes, then I'd suggest just warning if you
> try to implicitly or explicitly map something use_device in outer
> contexts, and just make sure you don't ICE on the cases where you
> warn. If the standard does not say what it means, then it is
> unspecified behavior...

A problem with deviceptr, unlike is_device_ptr, is that it turns out to
be defined only to work with pointers, not arrays (OpenACC 2.0a
2.6.5.2), and there are no rules describing the latter decaying to the
former. So at least if 'x' is an array, it appears the answer is "no".

So, the attached patch disallows (via raising an error):

* Variables being declared in explicit mapping clauses that are
  declared in enclosing host_data regions.

* Variables being implicitly used (mapped) in offloaded regions that
  are declared in enclosing host_data regions.

It's otherwise equivalent to the previously-posted version, but without
the hacks to {maybe_,}lookup_decl_in_outer_ctx. I added checks for the
above conditions during gimplification, which seemed to be about the
same phase that other similar kinds of errors are diagnosed.

Tests look OK (libgomp/gcc/g++/libstdc++), and the new ones pass.

OK for mainline?

Thanks,

Julian

ChangeLog

Julian Brown  
Cesar Philippidis  
James Norris  

gcc/
* c-family/c-pragma.c (oacc_pragmas): Add PRAGMA_OACC_HOST_DATA.
* c-family/c-pragma.h (pragma_kind): Add PRAGMA_OACC_HOST_DATA.
(pragma_omp_clause): Add PRAGMA_OACC_CLAUSE_USE_DEVICE.
* c/c-parser.c (c_parser_omp_clause_name): Add use_device support.
(c_parser_oacc_clause_use_device): New function.
(c_parser_oacc_all_clauses): Add use_device support.
(OACC_HOST_DATA_CLAUSE_MASK): New macro.
(c_parser_oacc_host_data): New function.
(c_parser_omp_construct): Add host_data support.
* c/c-tree.h (c_finish_oacc_host_data): Add prototype.
* c/c-typeck.c (c_finish_oacc_host_data): New function.
(c_finish_omp_clauses): Add use_device support.
* cp/cp-tree.h (finish_oacc_host_data): Add prototype.
* cp/parser.c (cp_parser_omp_clause_name): Add use_device support.
(cp_parser_oacc_all_clauses): Add use_device support.
(OACC_HOST_DATA_CLAUSE_MASK): New macro.
(cp_parser_oacc_host_data): New function.
(cp_parser_omp_construct): Add host_data support.
(cp_parser_pragma): Add host_data support.
* cp/semantics.c (finish_omp_clauses): Add use_device support.
(finish_oacc_host_data): New function.
* gimple-pretty-print.c (dump_gimple_omp_target): Add host_data
support.
* gimple.h (gf_mask): Add GF_OMP_TARGET_KIND_OACC_HOST_DATA.
(is_gimple_omp_oacc): Add support for above.
* gimplify.c (omp_region_type): Add ORT_ACC_HOST_DATA.
(omp_notice_variable): Diagnose undefined implicit uses of
use_device variables in offloaded regions.
(gimplify_scan_omp_clauses): Add host_data, use_device
support. Diagnose undefined mapping of use_device variables in
OpenACC clauses.
(gimplify_omp_workshare): Add host_data support.
(gimplify_expr): Likewise.
* omp-builtins.def (BUILT_IN_GOACC_HOST_DATA): New.
* omp-low.c (lookup_decl_in_outer_ctx)
(maybe_lookup_decl_in_outer_ctx): Add optional argument to skip
host_data regions.
(scan_sharing_clauses): Support use_device.
(check_omp_nesting_restrictions): Support host_data.
(expand_omp_target): Support host_data.
(lower_omp_target): Skip over outer host_data regions when looking
up decls. Support use_device.
(make_gimple_omp_edges): Support host_data.
* tree-nested.c (convert_nonlocal_omp_clauses): Add use_device
clause.

libgomp/
* oacc-parallel.c (GOACC_host_data): New function.
* libgomp.map (GOACC_host_data): Add to GOACC_2.0.1.
* testsuite/libgomp.oacc-c-c++-common/host_data-1.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-2.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-3.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-4.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-5.c: New test.
* testsui

Re: [OpenACC 0/7] host_data construct

[Jakub Jelinek]

On Mon, Nov 30, 2015 at 07:30:34PM +, Julian Brown wrote:
> Julian Brown  
> Cesar Philippidis  
> James Norris  
> 
> gcc/
> * c-family/c-pragma.c (oacc_pragmas): Add PRAGMA_OACC_HOST_DATA.
> * c-family/c-pragma.h (pragma_kind): Add PRAGMA_OACC_HOST_DATA.

c-family/, c/ and cp/ subdirectories have their own ChangeLog, so you need
to split the entry into multiple ChangeLog files and remove the directory
prefixes.

> @@ -6120,6 +6121,9 @@ omp_notice_variable (struct gimplify_omp_ctx *ctx, tree 
> decl, bool in_code)
>(splay_tree_key) decl);
> if (n2)
>   {
> +   if (octx->region_type == ORT_ACC_HOST_DATA)
> + error ("variable %qE declared in enclosing "
> +"host_data region", DECL_NAME (decl));

% instead?
> nflags |= GOVD_MAP;
> goto found_outer;
>   }
> @@ -6418,6 +6422,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
> *pre_p,
>case OMP_TARGET_DATA:
>case OMP_TARGET_ENTER_DATA:
>case OMP_TARGET_EXIT_DATA:
> +  case OACC_HOST_DATA:
>   ctx->target_firstprivatize_array_bases = true;
>default:
>   break;
> @@ -6683,6 +6688,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
> *pre_p,
>   case OMP_TARGET_DATA:
>   case OMP_TARGET_ENTER_DATA:
>   case OMP_TARGET_EXIT_DATA:
> + case OACC_HOST_DATA:
> if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
> || (OMP_CLAUSE_MAP_KIND (c)
> == GOMP_MAP_FIRSTPRIVATE_REFERENCE))
> @@ -6695,6 +6701,22 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
> *pre_p,
>   }
> if (remove)
>   break;
> +   if (DECL_P (decl) && outer_ctx && (region_type & ORT_ACC))
> + {
> +   struct gimplify_omp_ctx *octx;
> +   for (octx = outer_ctx; octx; octx = octx->outer_context)
> + {
> +   if (!(octx->region_type & (ORT_TARGET_DATA | ORT_TARGET)))
> + break;

Wouldn't it be better to do
if (octx->region_type != ORT_ACC_HOST_DATA)
  continue;
here, thus only lookup if you really want to use it?

> +   splay_tree_node n2
> + = splay_tree_lookup (octx->variables,
> +  (splay_tree_key) decl);
> +   if (n2 && octx->region_type == ORT_ACC_HOST_DATA)

and remove the && ... part from the condition?

> + error_at (OMP_CLAUSE_LOCATION (c), "variable %qE "
> +   "declared in enclosing host_data region",
> +   DECL_NAME (decl));
> + }
> + }
> if (OMP_CLAUSE_SIZE (c) == NULL_TREE)
>   OMP_CLAUSE_SIZE (c) = DECL_P (decl) ? DECL_SIZE_UNIT (decl)
> : TYPE_SIZE_UNIT (TREE_TYPE (decl));

Ok with those changes.

Jakub

Re: [OpenACC 0/7] host_data construct

[Tom de Vries]

On 30/11/15 20:30, Julian Brown wrote:

 libgomp/
 * oacc-parallel.c (GOACC_host_data): New function.
 * libgomp.map (GOACC_host_data): Add to GOACC_2.0.1.
 * testsuite/libgomp.oacc-c-c++-common/host_data-1.c: New test.
 * testsuite/libgomp.oacc-c-c++-common/host_data-2.c: New test.
 * testsuite/libgomp.oacc-c-c++-common/host_data-3.c: New test.
 * testsuite/libgomp.oacc-c-c++-common/host_data-4.c: New test.
 * testsuite/libgomp.oacc-c-c++-common/host_data-5.c: New test.
 * testsuite/libgomp.oacc-c-c++-common/host_data-6.c: New test.



Hi,

At r231169, I'm seeing these failures for a no-accelerator setup:
...
FAIL: libgomp.oacc-c/../libgomp.oacc-c-c++-common/host_data-2.c 
-DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 execution test
FAIL: libgomp.oacc-c/../libgomp.oacc-c-c++-common/host_data-4.c 
-DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 execution test
FAIL: libgomp.oacc-c/../libgomp.oacc-c-c++-common/host_data-5.c 
-DACC_DEVICE_TYPE_host=1 -DACC_MEM_SHARED=1 execution test

...

Thanks,
- Tom

Re: [OpenACC 0/7] host_data construct

[Thomas Schwinge]

Hi!

Cesar and Jim copied, for help with Fortran and generally testsuite
things.

On Mon, 30 Nov 2015 19:30:34 +, Julian Brown  
wrote:
> [patch]

First, thanks!

> Tests look OK (libgomp/gcc/g++/libstdc++), and the new ones pass.

I see a regression (ICE) in gfortran.dg/goacc/coarray.f95 (done: XFAILed,
and obsolete dg-excess-errors directives removed; compare to
gfortran.dg/goacc/coarray_2.f90), and I see new FAILs for non-offloading
execution of libgomp.oacc-c-c++-common/host_data-2.c,
libgomp.oacc-c-c++-common/host_data-4.c, and
libgomp.oacc-c-c++-common/host_data-5.c (done: see below); confirmed by a
number of reports on the  and
 mailing lists.  I can understand that you
didn't see the Fortran problem if not running Fortrant testing (but
why?), but it's strange that you didn't see the libgomp C/C++ FAILs.

A few patch review items, some of which I've already addressed (see
below).

> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -10279,6 +10279,8 @@ c_parser_omp_clause_name (c_parser *parser)
>   result = PRAGMA_OMP_CLAUSE_UNTIED;
> else if (!strcmp ("use_device_ptr", p))
>   result = PRAGMA_OMP_CLAUSE_USE_DEVICE_PTR;
> +   else if (!strcmp ("use_device", p))
> + result = PRAGMA_OACC_CLAUSE_USE_DEVICE;

"use_device" sorts before "use_device_ptr".  (Done.)

> @@ -12940,6 +12951,10 @@ c_parser_oacc_all_clauses (c_parser *parser, 
> omp_clause_mask mask,
> clauses = c_parser_oacc_data_clause (parser, c_kind, clauses);
> c_name = "self";
> break;
> + case PRAGMA_OACC_CLAUSE_USE_DEVICE:
> +   clauses = c_parser_oacc_clause_use_device (parser, clauses);
> +   c_name = "use_device";
> +   break;
>   case PRAGMA_OACC_CLAUSE_SEQ:
> clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_SEQ,
>   clauses);

Sorting?  (Done.)

> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -29232,6 +29232,8 @@ cp_parser_omp_clause_name (cp_parser *parser)
>   result = PRAGMA_OMP_CLAUSE_UNTIED;
> else if (!strcmp ("use_device_ptr", p))
>   result = PRAGMA_OMP_CLAUSE_USE_DEVICE_PTR;
> +   else if (!strcmp ("use_device", p))
> + result = PRAGMA_OACC_CLAUSE_USE_DEVICE;
> break;

Likewise.  (Done.)

> @@ -31598,6 +31600,11 @@ cp_parser_oacc_all_clauses (cp_parser *parser, 
> omp_clause_mask mask,
> clauses = cp_parser_oacc_data_clause (parser, c_kind, clauses);
> c_name = "self";
> break;
> + case PRAGMA_OACC_CLAUSE_USE_DEVICE:
> +   clauses = cp_parser_omp_var_list (parser, OMP_CLAUSE_USE_DEVICE,
> + clauses);
> +   c_name = "use_device";
> +   break;
>   case PRAGMA_OACC_CLAUSE_SEQ:
> clauses = cp_parser_oacc_simple_clause (parser, OMP_CLAUSE_SEQ,
>clauses, here);

Likewise.  (Done.)

> +#define OACC_HOST_DATA_CLAUSE_MASK   \
> +  ( (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_USE_DEVICE) )
> +
> +/* OpenACC 2.0:
> +  # pragma acc host_data  new-line
> +  structured-block  */

Define OACC_HOST_DATA_CLAUSE_MASK after the "accepted syntax" comment.
(Done.)

There is no handlig of OMP_CLAUSE_USE_DEVICE in
gcc/cp/pt.c:tsubst_omp_clauses.  (Done.)

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c

> @@ -6418,6 +6422,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
> *pre_p,
|if (!lang_GNU_Fortran ())
|  switch (code)
|{
|case OMP_TARGET:
>case OMP_TARGET_DATA:
>case OMP_TARGET_ENTER_DATA:
>case OMP_TARGET_EXIT_DATA:
> +  case OACC_HOST_DATA:
>   ctx->target_firstprivatize_array_bases = true;
>default:
>   break;

I understand it's not yet relevant/supported for OpenMP in Fortran, but
why is C/C++ vs. Fortran being handled differently here for OpenACC
host_data?

> --- a/libgomp/oacc-parallel.c
> +++ b/libgomp/oacc-parallel.c

> +void
> +GOACC_host_data (int device, size_t mapnum,
> +  void **hostaddrs, size_t *sizes, unsigned short *kinds)
> +{
> +  bool host_fallback = device == GOMP_DEVICE_HOST_FALLBACK;
> +  struct target_mem_desc *tgt;
> +
> +#ifdef HAVE_INTTYPES_H
> +  gomp_debug (0, "%s: mapnum=%"PRIu64", hostaddrs=%p, size=%p, kinds=%p\n",
> +   __FUNCTION__, (uint64_t) mapnum, hostaddrs, sizes, kinds);
> +#else
> +  gomp_debug (0, "%s: mapnum=%lu, hostaddrs=%p, sizes=%p, kinds=%p\n",
> +   __FUNCTION__, (unsigned long) mapnum, hostaddrs, sizes, kinds);
> +#endif
> +
> +  goacc_lazy_initialize ();
> +
> +  struct goacc_thread *thr = goacc_thread ();
> +  struct gomp_device_descr *acc_dev = thr->dev;
> +
> +  /* Host fallback or 'do nothing'.  */
> +  if ((acc_dev->capabilities & GOMP_OFFLOAD_CAP_SHARED_MEM)
> +  || host_fallback)
> +{
> +  tgt = gomp_map_vars (NULL, 0, NULL, NULL, NULL, NULL, true,
> +GOMP_MAP_VARS_OPENACC);
> +  tgt

Re: [OpenACC 0/7] host_data construct

[Cesar Philippidis]

On 12/02/2015 07:58 AM, Thomas Schwinge wrote:

> diff --git gcc/testsuite/gfortran.dg/goacc/coarray.f95 
> gcc/testsuite/gfortran.dg/goacc/coarray.f95
> index 130ffc3..d2f10d5 100644
> --- gcc/testsuite/gfortran.dg/goacc/coarray.f95
> +++ gcc/testsuite/gfortran.dg/goacc/coarray.f95
> @@ -1,7 +1,9 @@
>  ! { dg-do compile } 
>  ! { dg-additional-options "-fcoarray=single" }
> -
> -! TODO: These cases must fail
> +!
> +! PR fortran/63861
> +! { dg-xfail-if "" { *-*-* } }
> +! { dg-excess-errors "TODO" }
>  
>  module test
>  contains
> @@ -9,7 +11,6 @@ contains
>  implicit none
>  integer :: i
>  integer, codimension[*] :: a
> -! { dg-excess-errors "sorry, unimplemented: directive not yet 
> implemented" }
>  !$acc declare device_resident (a)
>  !$acc data copy (a)
>  !$acc end data
> @@ -17,7 +18,6 @@ contains
>  !$acc end data
>  !$acc parallel private (a)
>  !$acc end parallel
> -! { dg-excess-errors "sorry, unimplemented: directive not yet 
> implemented" }
>  !$acc host_data use_device (a)
>  !$acc end host_data
>  !$acc parallel loop reduction(+:a)
> diff --git gcc/testsuite/gfortran.dg/goacc/coarray_2.f90 
> gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
> index f9cf9ac..87e04d5 100644
> --- gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
> +++ gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
> @@ -3,6 +3,7 @@
>  !
>  ! PR fortran/63861
>  ! { dg-xfail-if "" { *-*-* } }
> +! { dg-excess-errors "TODO" }

This host_data patch exposed a bug in the fortran front end where it was
allowing arrays to be used as reduction variables. If replace you
replace codimension with dimension, you'd see a similar ICE. The
attached patch, while it doesn't make any attempt to fix the gimplifier
changes, does teach the fortran front end to error on acc reductions
containing array variables.

Note that this solution is somewhat aggressive because we probably
should allow reductions on individual array elements. E.g.

  !$acc loop reduction(+:var(1))

The c and c++ front ends also have that problem. Maybe I'll revisit this
later.

Is this ok for trunk? It will close pr63861.

Cesar
2015-12-02  Cesar Philippidis  

	gcc/fortran/
	PR fortran/63861
	* openmp.c (gfc_match_omp_clauses): Allow subarrays for acc reductions.
	(resolve_omp_clauses): Error on any acc reductions on arrays.

	gcc/testsuite/
	* gfortran.dg/goacc/array-reduction.f90: New test.
	* gfortran.dg/goacc/assumed.f95: Update expected diagnostics.
	* gfortran.dg/goacc/coarray.f95: Likewise.
	* gfortran.dg/goacc/coarray_2.f90: Likewise.
	* gfortran.dg/goacc/reduction-2.f95: Likewise.
	* gfortran.dg/goacc/reduction.f95: Likewise.

diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 6182464..276f2f1 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -978,7 +978,8 @@ gfc_match_omp_clauses (gfc_omp_clauses **cp, uint64_t mask,
 
 	  if (gfc_match_omp_variable_list (" :",
 	   &c->lists[OMP_LIST_REDUCTION],
-	   false, NULL, &head) == MATCH_YES)
+	   false, NULL, &head, openacc)
+	  == MATCH_YES)
 	{
 	  gfc_omp_namelist *n;
 	  if (rop == OMP_REDUCTION_NONE)
@@ -3313,6 +3314,11 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 		   n->sym->name, &n->where);
 	  else
 	n->sym->mark = 1;
+
+	  /* OpenACC does not support reductions on arrays.  */
+	  if (n->sym->as)
+	gfc_error ("Array %qs is not permitted in reduction at %L",
+		   n->sym->name, &n->where);
 	}
 }
   
diff --git a/gcc/testsuite/gfortran.dg/goacc/array-reduction.f90 b/gcc/testsuite/gfortran.dg/goacc/array-reduction.f90
new file mode 100644
index 000..d71c400
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/array-reduction.f90
@@ -0,0 +1,74 @@
+program test
+  implicit none
+  integer a(10), i
+
+  a(:) = 0
+  
+  ! Array reductions.
+  
+  !$acc parallel reduction (+:a) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end parallel
+
+  !$acc parallel
+  !$acc loop reduction (+:a) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end parallel
+
+  !$acc kernels
+  !$acc loop reduction (+:a) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end kernels
+
+  ! Subarray reductions.
+  
+  !$acc parallel reduction (+:a(1:5)) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end parallel
+
+  !$acc parallel
+  !$acc loop reduction (+:a(1:5)) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end parallel
+
+  !$acc kernels
+  !$acc loop reduction (+:a(1:5)) ! { dg-error "Array 'a' is not permitted in reduction" }
+  do i = 1, 10
+ a = a + 1
+  end do
+  !$acc end kernels
+
+  ! Reductions on array elements.
+  
+  !$acc

Re: [OpenACC 0/7] host_data construct

[Steve Kargl]

On Wed, Dec 02, 2015 at 11:16:10AM -0800, Cesar Philippidis wrote:
> 
> This host_data patch exposed a bug in the fortran front end where it was
> allowing arrays to be used as reduction variables. If replace you
> replace codimension with dimension, you'd see a similar ICE. The
> attached patch, while it doesn't make any attempt to fix the gimplifier
> changes, does teach the fortran front end to error on acc reductions
> containing array variables.
> 
> Note that this solution is somewhat aggressive because we probably
> should allow reductions on individual array elements. E.g.
> 
>   !$acc loop reduction(+:var(1))
> 
> The c and c++ front ends also have that problem. Maybe I'll revisit this
> later.
> 
> Is this ok for trunk? It will close pr63861.
> 

I think that it is OK, but will defer to Jakub or Thomas.
I suspect tht Jakub may be pre-occupied with the upcoming
5.3 release.

-- 
Steve

Re: [OpenACC 0/7] host_data construct

[Jakub Jelinek]

On Wed, Dec 02, 2015 at 11:16:10AM -0800, Cesar Philippidis wrote:
> > --- gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
> > +++ gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
> > @@ -3,6 +3,7 @@
> >  !
> >  ! PR fortran/63861
> >  ! { dg-xfail-if "" { *-*-* } }
> > +! { dg-excess-errors "TODO" }
> 
> This host_data patch exposed a bug in the fortran front end where it was
> allowing arrays to be used as reduction variables. If replace you
> replace codimension with dimension, you'd see a similar ICE. The
> attached patch, while it doesn't make any attempt to fix the gimplifier
> changes, does teach the fortran front end to error on acc reductions
> containing array variables.

Does the OpenACC standard disallow array reductions?
Just asking, because OpenMP allows them (up to 4.0 only in Fortran,
in 4.5 also C/C++ array sections are allowed).

If the OpenACC standard disallows them, then it is desirable to reject them
and the patch is ok, otherwise you should try harder to support them ;).

Jakub

Re: [OpenACC 0/7] host_data construct

[Cesar Philippidis]

On 12/02/2015 11:35 AM, Jakub Jelinek wrote:
> On Wed, Dec 02, 2015 at 11:16:10AM -0800, Cesar Philippidis wrote:
>>> --- gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
>>> +++ gcc/testsuite/gfortran.dg/goacc/coarray_2.f90
>>> @@ -3,6 +3,7 @@
>>>  !
>>>  ! PR fortran/63861
>>>  ! { dg-xfail-if "" { *-*-* } }
>>> +! { dg-excess-errors "TODO" }
>>
>> This host_data patch exposed a bug in the fortran front end where it was
>> allowing arrays to be used as reduction variables. If replace you
>> replace codimension with dimension, you'd see a similar ICE. The
>> attached patch, while it doesn't make any attempt to fix the gimplifier
>> changes, does teach the fortran front end to error on acc reductions
>> containing array variables.
> 
> Does the OpenACC standard disallow array reductions?
> Just asking, because OpenMP allows them (up to 4.0 only in Fortran,
> in 4.5 also C/C++ array sections are allowed).
> 
> If the OpenACC standard disallows them, then it is desirable to reject them
> and the patch is ok, otherwise you should try harder to support them ;).

Array reductions aren't supported in OpenACC 2.0.

Cesar

Re: [OpenACC 0/7] host_data construct

[Joseph Myers]

I think this patch is small enough, and the pieces insufficiently 
self-contained, that splitting it up rather than posting as one patch just 
makes it harder to understand.  My strong preference is that the same 
patch that introduces a feature should also add the testcases for that 
feature, for example - they should not be split out (that's not even a 
split by reviewer, testcases are critical to reviewing functionality 
patches).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [OpenACC 0/7] host_data construct

[James Norris]

To all,

On 10/22/2015 03:36 PM, Joseph Myers wrote:

I think this patch is small enough, and the pieces insufficiently
self-contained, that splitting it up rather than posting as one patch just
makes it harder to understand.  My strong preference is that the same
patch that introduces a feature should also add the testcases for that
feature, for example - they should not be split out (that's not even a
split by reviewer, testcases are critical to reviewing functionality
patches).



Okay, I'll rewrite the posting and submit it with a single patch
as a response to the initial posting. I'll also re-write the comments
as they have been pointed out by Nathan as being too terse.

My apologies for wasting people's time.
Jim

Re: [OpenACC 0/7] host_data construct

[Julian Brown]

On Mon, 2 Nov 2015 18:33:39 +
Julian Brown  wrote:

> On Mon, 26 Oct 2015 19:34:22 +0100
> Jakub Jelinek  wrote:
> 
> > Your use_device sounds very similar to use_device_ptr clause in
> > OpenMP, which is allowed on #pragma omp target data construct and is
> > implemented quite a bit differently from this; it is unclear if the
> > OpenACC standard requires this kind of implementation, or you just
> > chose to implement it this way.  In particular, the GOMP_target_data
> > call puts the variables mentioned in the use_device_ptr clauses into
> > the mapping structures (similarly how map clause appears) and the
> > corresponding vars are privatized within the target data region
> > (which is a host region, basically a fancy { } braces), where the
> > private variables contain the offloading device's pointers.  
> 
> As the author of the original patch, I have to say using the mapping
> structures seems like a far better approach, but I've hit some trouble
> with the details of adapting OpenACC to use that method.

Here's a version of the patch which (hopefully) brings OpenACC on par
with OpenMP with respect to use_device/use_device_ptr variables. The
implementation is essentially the same now for OpenACC as for OpenMP
(i.e. using mapping structures): so for now, only array or pointer
variables can be used as use_device variables. The included tests have
been adjusted accordingly.

One awkward part of the implementation concerns nesting offloaded
regions within host_data regions:

#define N 1024

int main (int argc, char* argv[])
{
  int x[N];

#pragma acc data copyin (x[0:N])
  {
int *xp;
#pragma acc host_data use_device (x)
{
  [...]
#pragma acc parallel present (x) copyout (xp)
  {
xp = x;
  }
}

assert (xp == acc_deviceptr (x));
  }

  return 0;
}

I think the meaning of 'x' as seen within the clauses of the parallel
directive should be the *host* version of x, not the mapped target
address (I've asked on the OpenACC technical mailing list to clarify
this point, but no reply as yet). The changes to
{maybe_,}lookup_decl_in_outer_ctx "skip over" host_data contexts when
called from lower_omp_target. There's probably an analogous case for
OpenMP, but I've not tried to handle that.

No regressions for libgomp tests, and the new tests pass. OK for trunk?

Thanks,

Julian

ChangeLog

Julian Brown  
Cesar Philippidis  
James Norris  

gcc/
* c-family/c-pragma.c (oacc_pragmas): Add PRAGMA_OACC_HOST_DATA.
* c-family/c-pragma.h (pragma_kind): Add PRAGMA_OACC_HOST_DATA.
(pragma_omp_clause): Add PRAGMA_OACC_CLAUSE_USE_DEVICE.
* c/c-parser.c (c_parser_omp_clause_name): Add use_device support.
(c_parser_oacc_clause_use_device): New function.
(c_parser_oacc_all_clauses): Add use_device support.
(OACC_HOST_DATA_CLAUSE_MASK): New macro.
(c_parser_oacc_host_data): New function.
(c_parser_omp_construct): Add host_data support.
* c/c-tree.h (c_finish_oacc_host_data): Add prototype.
* c/c-typeck.c (c_finish_oacc_host_data): New function.
(c_finish_omp_clauses): Add use_device support.
* cp/cp-tree.h (finish_oacc_host_data): Add prototype.
* cp/parser.c (cp_parser_omp_clause_name): Add use_device support.
(cp_parser_oacc_all_clauses): Add use_device support.
(OACC_HOST_DATA_CLAUSE_MASK): New macro.
(cp_parser_oacc_host_data): New function.
(cp_parser_omp_construct): Add host_data support.
(cp_parser_pragma): Add host_data support.
* cp/semantics.c (finish_omp_clauses): Add use_device support.
(finish_oacc_host_data): New function.
* gimple-pretty-print.c (dump_gimple_omp_target): Add host_data
support.
* gimple.h (gf_mask): Add GF_OMP_TARGET_KIND_OACC_HOST_DATA.
(is_gimple_omp_oacc): Add support for above.
* gimplify.c (gimplify_scan_omp_clauses): Add host_data, use_device
support.
(gimplify_omp_workshare): Add host_data support.
(gimplify_expr): Likewise.
* omp-builtins.def (BUILT_IN_GOACC_HOST_DATA): New.
* omp-low.c (lookup_decl_in_outer_ctx)
(maybe_lookup_decl_in_outer_ctx): Add optional argument to skip
host_data regions.
(scan_sharing_clauses): Support use_device.
(check_omp_nesting_restrictions): Support host_data.
(expand_omp_target): Support host_data.
(lower_omp_target): Skip over outer host_data regions when looking
up decls. Support use_device.
(make_gimple_omp_edges): Support host_data.
* tree-nested.c (convert_nonlocal_omp_clauses): Add use_device
clause.

libgomp/
* oacc-parallel.c (GOACC_host_data): New function.
* libgomp.map (GOACC_host_data): Add to GOACC_2.0.1.
* testsuite/libgomp.oacc-c-c++-common/host_data-1.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-2.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-3.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-4.c: New test.
* testsuite/libgomp.oacc-c-c++-common/host_data-5.c: N

Re: [OpenACC 0/7] host_data construct

[Julian Brown]

On Thu, 12 Nov 2015 11:16:21 +
Julian Brown  wrote:

> Here's a version of the patch which (hopefully) brings OpenACC on par
> with OpenMP with respect to use_device/use_device_ptr variables. The
> implementation is essentially the same now for OpenACC as for OpenMP
> (i.e. using mapping structures): so for now, only array or pointer
> variables can be used as use_device variables. The included tests have
> been adjusted accordingly.

Here's a rebased version of the patch, since the previous version no
longer applies cleanly. Re-tested OK (libgomp tests). ChangeLog as
before. (Ping.)

Juliancommit 0201a5927c380da65d6400afad4a0e277fb85786
Author: Julian Brown 
Date:   Mon Nov 2 06:31:47 2015 -0800

OpenACC host_data support using mapping regions.

diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 12c3e75..56cf697 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1251,6 +1251,7 @@ static const struct omp_pragma_def oacc_pragmas[] = {
   { "declare", PRAGMA_OACC_DECLARE },
   { "enter", PRAGMA_OACC_ENTER_DATA },
   { "exit", PRAGMA_OACC_EXIT_DATA },
+  { "host_data", PRAGMA_OACC_HOST_DATA },
   { "kernels", PRAGMA_OACC_KERNELS },
   { "loop", PRAGMA_OACC_LOOP },
   { "parallel", PRAGMA_OACC_PARALLEL },
diff --git a/gcc/c-family/c-pragma.h b/gcc/c-family/c-pragma.h
index 999ac67..dd246b9 100644
--- a/gcc/c-family/c-pragma.h
+++ b/gcc/c-family/c-pragma.h
@@ -33,6 +33,7 @@ enum pragma_kind {
   PRAGMA_OACC_DECLARE,
   PRAGMA_OACC_ENTER_DATA,
   PRAGMA_OACC_EXIT_DATA,
+  PRAGMA_OACC_HOST_DATA,
   PRAGMA_OACC_KERNELS,
   PRAGMA_OACC_LOOP,
   PRAGMA_OACC_PARALLEL,
@@ -167,6 +168,7 @@ enum pragma_omp_clause {
   PRAGMA_OACC_CLAUSE_SELF,
   PRAGMA_OACC_CLAUSE_SEQ,
   PRAGMA_OACC_CLAUSE_TILE,
+  PRAGMA_OACC_CLAUSE_USE_DEVICE,
   PRAGMA_OACC_CLAUSE_VECTOR,
   PRAGMA_OACC_CLAUSE_VECTOR_LENGTH,
   PRAGMA_OACC_CLAUSE_WAIT,
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 7b10764..0a5c8bb 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -10267,6 +10267,8 @@ c_parser_omp_clause_name (c_parser *parser)
 	result = PRAGMA_OMP_CLAUSE_UNTIED;
 	  else if (!strcmp ("use_device_ptr", p))
 	result = PRAGMA_OMP_CLAUSE_USE_DEVICE_PTR;
+	  else if (!strcmp ("use_device", p))
+	result = PRAGMA_OACC_CLAUSE_USE_DEVICE;
 	  break;
 	case 'v':
 	  if (!strcmp ("vector", p))
@@ -11619,6 +11621,15 @@ c_parser_oacc_clause_tile (c_parser *parser, tree list)
   return c;
 }
 
+/* OpenACC 2.0:
+   use_device ( variable-list ) */
+
+static tree
+c_parser_oacc_clause_use_device (c_parser *parser, tree list)
+{
+  return c_parser_omp_var_list_parens (parser, OMP_CLAUSE_USE_DEVICE, list);
+}
+
 /* OpenACC:
wait ( int-expr-list ) */
 
@@ -12928,6 +12939,10 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
 	  clauses = c_parser_oacc_data_clause (parser, c_kind, clauses);
 	  c_name = "self";
 	  break;
+	case PRAGMA_OACC_CLAUSE_USE_DEVICE:
+	  clauses = c_parser_oacc_clause_use_device (parser, clauses);
+	  c_name = "use_device";
+	  break;
 	case PRAGMA_OACC_CLAUSE_SEQ:
 	  clauses = c_parser_oacc_simple_clause (parser, OMP_CLAUSE_SEQ,
 		clauses);
@@ -13577,6 +13592,29 @@ c_parser_oacc_enter_exit_data (c_parser *parser, bool enter)
 
 
 /* OpenACC 2.0:
+   # pragma acc host_data oacc-data-clause[optseq] new-line
+ structured-block
+*/
+
+#define OACC_HOST_DATA_CLAUSE_MASK	\
+	( (OMP_CLAUSE_MASK_1 << PRAGMA_OACC_CLAUSE_USE_DEVICE) )
+
+static tree
+c_parser_oacc_host_data (location_t loc, c_parser *parser)
+{
+  tree stmt, clauses, block;
+
+  clauses = c_parser_oacc_all_clauses (parser, OACC_HOST_DATA_CLAUSE_MASK,
+   "#pragma acc host_data");
+
+  block = c_begin_omp_parallel ();
+  add_stmt (c_parser_omp_structured_block (parser));
+  stmt = c_finish_oacc_host_data (loc, clauses, block);
+  return stmt;
+}
+
+
+/* OpenACC 2.0:
 
# pragma acc loop oacc-loop-clause[optseq] new-line
  structured-block
@@ -16884,6 +16922,9 @@ c_parser_omp_construct (c_parser *parser)
 case PRAGMA_OACC_DATA:
   stmt = c_parser_oacc_data (loc, parser);
   break;
+case PRAGMA_OACC_HOST_DATA:
+  stmt = c_parser_oacc_host_data (loc, parser);
+  break;
 case PRAGMA_OACC_KERNELS:
 case PRAGMA_OACC_PARALLEL:
   strcpy (p_name, "#pragma acc");
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 6bc216a..848131e 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -653,6 +653,7 @@ extern tree c_finish_goto_ptr (location_t, tree);
 extern tree c_expr_to_decl (tree, bool *, bool *);
 extern tree c_finish_omp_construct (location_t, enum tree_code, tree, tree);
 extern tree c_finish_oacc_data (location_t, tree, tree);
+extern tree c_finish_oacc_host_data (location_t, tree, tree);
 extern tree c_begin_omp_parallel (void);
 extern tree c_finish_omp_parallel (location_t, tree, tree);
 extern tree c_begin_omp_task (void);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index c18c307..837775b 100644
--- a/gcc/c/c-typeck.c
+++ b/

Re: [OpenACC 0/7] host_data construct

[Jakub Jelinek]

On Wed, Nov 18, 2015 at 12:47:47PM +, Julian Brown wrote:

The FE/gimplifier part is okay, but I really don't like the
omp-low.c changes, mostly the *lookup_decl_in_outer_ctx* changes.
If I count well, we have right now 27 maybe_lookup_decl_in_outer_ctx
callers and 7 lookup_decl_in_outer_ctx callers, you want to change
behavior of 1 maybe_lookup_decl_in_outer_ctx and 1
lookup_decl_in_outer_ctx.  Why exactly those 2 and not the others?
What are the exact rules (what does the standard say about it)?
I'd expect that all phases (scan_sharing_clauses, lower_omp* and
expand_omp*) should agree on the same behavior, otherwise I can't see how it
can work properly.  And, if you want to change just a couple of spots,
I'd strongly prefer to add new functions with this weirdo behavior, rather
than tweaking the original function.

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -390,8 +390,8 @@ scan_omp_op (tree *tp, omp_context *ctx)
>  }
>  
>  static void lower_omp (gimple_seq *, omp_context *);
> -static tree lookup_decl_in_outer_ctx (tree, omp_context *);
> -static tree maybe_lookup_decl_in_outer_ctx (tree, omp_context *);
> +static tree lookup_decl_in_outer_ctx (tree, omp_context *, bool = false);
> +static tree maybe_lookup_decl_in_outer_ctx (tree, omp_context *, bool = 
> false);
>  
>  /* Find an OMP clause of type KIND within CLAUSES.  */
>  
> @@ -1935,6 +1935,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> install_var_local (decl, ctx);
> break;
>  
> + case OMP_CLAUSE_USE_DEVICE:
>   case OMP_CLAUSE_USE_DEVICE_PTR:
> decl = OMP_CLAUSE_DECL (c);
> if (TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE)
> @@ -2137,7 +2138,6 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> break;
>  
>   case OMP_CLAUSE_DEVICE_RESIDENT:
> - case OMP_CLAUSE_USE_DEVICE:
>   case OMP_CLAUSE__CACHE_:
> sorry ("Clause not supported yet");
> break;
> @@ -2288,6 +2288,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
>   case OMP_CLAUSE_SIMD:
>   case OMP_CLAUSE_NOGROUP:
>   case OMP_CLAUSE_DEFAULTMAP:
> + case OMP_CLAUSE_USE_DEVICE:
>   case OMP_CLAUSE_USE_DEVICE_PTR:
>   case OMP_CLAUSE__CILK_FOR_COUNT_:
>   case OMP_CLAUSE_ASYNC:
> @@ -2305,7 +2306,6 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> break;
>  
>   case OMP_CLAUSE_DEVICE_RESIDENT:
> - case OMP_CLAUSE_USE_DEVICE:
>   case OMP_CLAUSE__CACHE_:
> sorry ("Clause not supported yet");
> break;
> @@ -3608,6 +3608,8 @@ check_omp_nesting_restrictions (gimple *stmt, 
> omp_context *ctx)
>   case GF_OMP_TARGET_KIND_OACC_UPDATE: stmt_name = "update"; break;
>   case GF_OMP_TARGET_KIND_OACC_ENTER_EXIT_DATA:
> stmt_name = "enter/exit data"; break;
> + case GF_OMP_TARGET_KIND_OACC_HOST_DATA: stmt_name = "host_data";
> +   break;
>   default: gcc_unreachable ();
>   }
> switch (gimple_omp_target_kind (ctx->stmt))
> @@ -3619,6 +3621,8 @@ check_omp_nesting_restrictions (gimple *stmt, 
> omp_context *ctx)
>   case GF_OMP_TARGET_KIND_OACC_KERNELS:
> ctx_stmt_name = "kernels"; break;
>   case GF_OMP_TARGET_KIND_OACC_DATA: ctx_stmt_name = "data"; break;
> + case GF_OMP_TARGET_KIND_OACC_HOST_DATA:
> +   ctx_stmt_name = "host_data"; break;
>   default: gcc_unreachable ();
>   }
>  
> @@ -3941,13 +3945,22 @@ maybe_lookup_ctx (gimple *stmt)
>  parallelism happens only rarely.  */
>  
>  static tree
> -lookup_decl_in_outer_ctx (tree decl, omp_context *ctx)
> +lookup_decl_in_outer_ctx (tree decl, omp_context *ctx,
> +   bool skip_hostdata)
>  {
>tree t;
>omp_context *up;
>  
>for (up = ctx->outer, t = NULL; up && t == NULL; up = up->outer)
> -t = maybe_lookup_decl (decl, up);
> +{
> +  if (skip_hostdata
> +   && gimple_code (up->stmt) == GIMPLE_OMP_TARGET
> +   && gimple_omp_target_kind (up->stmt)
> +  == GF_OMP_TARGET_KIND_OACC_HOST_DATA)
> + continue;
> +
> +  t = maybe_lookup_decl (decl, up);
> +}
>  
>gcc_assert (!ctx->is_nested || t || is_global_var (decl));
>  
> @@ -3959,13 +3972,22 @@ lookup_decl_in_outer_ctx (tree decl, omp_context *ctx)
> in outer contexts.  */
>  
>  static tree
> -maybe_lookup_decl_in_outer_ctx (tree decl, omp_context *ctx)
> +maybe_lookup_decl_in_outer_ctx (tree decl, omp_context *ctx,
> + bool skip_hostdata)
>  {
>tree t = NULL;
>omp_context *up;
>  
>for (up = ctx->outer, t = NULL; up && t == NULL; up = up->outer)
> -t = maybe_lookup_decl (decl, up);
> +{
> +  if (skip_hostdata
> +   && gimple_code (up->stmt) == GIMPLE_OMP_TARGET
> +   && gimple_omp_target_kind (up->stmt)
> +  == GF_OMP_TARGET_KIND_OACC_HOST_DATA)
> + continue;
> +
> +  t = maybe_lookup_decl (decl, up);
> +}
>  
>return t ? t : de

Re: [OpenACC 0/7] host_data construct

[Julian Brown]

On Thu, 19 Nov 2015 14:13:45 +0100
Jakub Jelinek  wrote:

> On Wed, Nov 18, 2015 at 12:47:47PM +, Julian Brown wrote:
> 
> The FE/gimplifier part is okay, but I really don't like the
> omp-low.c changes, mostly the *lookup_decl_in_outer_ctx* changes.
> If I count well, we have right now 27 maybe_lookup_decl_in_outer_ctx
> callers and 7 lookup_decl_in_outer_ctx callers, you want to change
> behavior of 1 maybe_lookup_decl_in_outer_ctx and 1
> lookup_decl_in_outer_ctx.  Why exactly those 2 and not the others?

The not-very-good reason is that those are the merely the places that
allowed the supplied examples to work, and I'm wary of changing other
code that I don't understand very well.

> What are the exact rules (what does the standard say about it)?
> I'd expect that all phases (scan_sharing_clauses, lower_omp* and
> expand_omp*) should agree on the same behavior, otherwise I can't see
> how it can work properly.

OK, thanks -- as to what the standard says, it's so ill-specified in
this area that nothing can be learned about the behaviour of offloaded
regions within host_data constructs, and my question about that on the
technical mailing list is still unanswered (actually Nathan suggested
in private mail that the conservative thing to do would be to disallow
offloaded regions entirely within host_data constructs, so maybe that's
the way to go).

OpenMP 4.5 seems to *not* specify the skipping-over behaviour for
use_device_ptr variables (p105, lines 20-23):

"The is_device_ptr clause is used to indicate that a list item is a
device pointer already in the device data environment and that it
should be used directly. Support for device pointers created outside
of OpenMP, specifically outside of the omp_target_alloc routine and the
use_device_ptr clause, is implementation defined."

That suggests that use_device_ptr is a valid way to create device
pointers for use in enclosed target regions: the behaviour I assumed
was wrong for OpenACC. So I think my guess at the "most-obvious"
behaviour was probably misguided anyway.

It's maybe even more complicated. Consider the example:

char x[1024];

#pragma acc enter data copyin(x)

#pragma acc host_data use_device(x)
{
  target_primitive(x);
  #pragma acc parallel present(x)[1]
  {
x[5] = 0;[2]
  }
}

Here, the "present" clause marked [1] will fail (because 'x' is a
target pointer now). If it's omitted, the array access [2] will cause an
implicit present_or_copy to be used for the 'x' pointer (which again
will fail, because now 'x' points to target data). Maybe what we
actually need is,

#pragma acc host_data use_device(x)
{
  target_primitive(x);
  #pragma acc parallel deviceptr(x)
  {
...
  }
}

with the deviceptr(x) clause magically substituted in the parallel
construct, but I'm struggling to see how we could justify doing that
when that behaviour's not mentioned in the spec at all.

Aha, so: maybe manually using deviceptr(x) is implicitly mandatory in
this situation, and missing it out should be an error? That suddenly
seems to make most sense. I'll see about fixing the patch to do that.

Julian

Re: [OpenACC 0/7] host_data construct

[Jakub Jelinek]

On Thu, Nov 19, 2015 at 02:26:50PM +, Julian Brown wrote:
> OK, thanks -- as to what the standard says, it's so ill-specified in
> this area that nothing can be learned about the behaviour of offloaded
> regions within host_data constructs, and my question about that on the
> technical mailing list is still unanswered (actually Nathan suggested
> in private mail that the conservative thing to do would be to disallow
> offloaded regions entirely within host_data constructs, so maybe that's
> the way to go).
> 
> OpenMP 4.5 seems to *not* specify the skipping-over behaviour for
> use_device_ptr variables (p105, lines 20-23):
> 
> "The is_device_ptr clause is used to indicate that a list item is a
> device pointer already in the device data environment and that it
> should be used directly. Support for device pointers created outside
> of OpenMP, specifically outside of the omp_target_alloc routine and the
> use_device_ptr clause, is implementation defined."
> 
> That suggests that use_device_ptr is a valid way to create device
> pointers for use in enclosed target regions: the behaviour I assumed
> was wrong for OpenACC. So I think my guess at the "most-obvious"
> behaviour was probably misguided anyway.

use_device_ptr kind of privatizes the variable, the private variable being
the device pointer corresponding to the host pointer outside of the target
data with use_device_ptr clause.

And, if you want to use that device pointer in a target region, it should be
on the is_device_ptr clause on the target construct.  See e.g.
libgomp.c/target-18.c testcase.
  int a[4];
...
  #pragma omp target data map(to:a)
  #pragma omp target data use_device_ptr(a) map(from:err)
  #pragma omp target is_device_ptr(a) private(i) map(from:err)
  {
err = 0;
for (i = 0; i < 4; i++)
  if (a[i] != 23 + i)
err = 1;
  }
The implementation has this way a choice how to implement device pointers
(what use_device_ptr gives you, or say omp_target_alloc returns)
- either (GCC's choice at least for the XeonPhi and hopefully PTX, HSA does
not care, as it shares address space) implement them as host pointer
encoding the bits the target device wants to use, or some kind of
descriptor.  In the former case, is_device_ptr is essentially a
firstprivate, you bitwise copy the device pointer from the host to target
device, where you can dereference it etc.  In the descriptor case you'd
do some transformation of the host side representation of the device pointer
to the device side.

> 
> It's maybe even more complicated. Consider the example:
> 
> char x[1024];
> 
> #pragma acc enter data copyin(x)
> 
> #pragma acc host_data use_device(x)
> {
>   target_primitive(x);
>   #pragma acc parallel present(x)[1]
>   {
> x[5] = 0;[2]
>   }
> }

If it is unclear, I think disallowing acc {parallel,kernels} inside of
acc host_data might be too big hammer, but perhaps just erroring out
or warning during gimplification that if you (explicitly or implicitly)
try to map a var that is in use_device clause in some outer context,
it is either wrong, unsupported or will not do what users think?

I will double check on omp-lang, but supposedly we could for OpenMP
warn in similar cases (use_device_ptr clause instead of use_device),
except when it is passed to is_device_ptr clause, because I think the
behavior is just unspecified otherwise.
> 
> Here, the "present" clause marked [1] will fail (because 'x' is a
> target pointer now). If it's omitted, the array access [2] will cause an
> implicit present_or_copy to be used for the 'x' pointer (which again
> will fail, because now 'x' points to target data). Maybe what we
> actually need is,
> 
> #pragma acc host_data use_device(x)
> {
>   target_primitive(x);
>   #pragma acc parallel deviceptr(x)
>   {
> ...
>   }
> }
> 
> with the deviceptr(x) clause magically substituted in the parallel
> construct, but I'm struggling to see how we could justify doing that
> when that behaviour's not mentioned in the spec at all.

Is deviceptr as above meant to work?  That is the OpenACC counterpart
of is_device_ptr, right?  If yes, then I'd suggest just warning if you
try to implicitly or explicitly map something use_device in outer contexts,
and just make sure you don't ICE on the cases where you warn.
If the standard does not say what it means, then it is unspecified
behavior...

Jakub

[gomp4] Re: [OpenACC 0/7] host_data construct

[Thomas Schwinge]

Hi!

On Wed, 2 Dec 2015 16:58:45 +0100, I wrote:
> Cesar and Jim copied, for help with Fortran and generally testsuite
> things.
> 
> On Mon, 30 Nov 2015 19:30:34 +, Julian Brown  
> wrote:
> > [patch]
> 
> First, thanks!

Aside from a number of formatting/re-ordering changes, the front end
changes were basically still the same, but otherwise (middle end,
libgomp) the patch as committed to trunk in r231118 was quite (totally?)
;-) different from the code we had on gomp-4_0-branch, so I had to spend
some time on merging, cleaning things up.

> What about the test cases present on gomp-4_0-branch,
> gcc/testsuite/c-c++-common/goacc/host_data-1.c,
> gcc/testsuite/c-c++-common/goacc/host_data-2.c,
> gcc/testsuite/c-c++-common/goacc/host_data-3.c, and
> gcc/testsuite/c-c++-common/goacc/host_data-4.c, [...]

In the merge, I had to move two use_device usages from
c-c++-common/goacc/host_data-1.c (was accepted) to
c-c++-common/goacc/host_data-2.c (now rejected); I hope that's correct.

> Your submission/commit didn't have any execution tests for OpenACC
> host_data in Fortran.  On gomp-4_0-branch, there is
> libgomp/testsuite/libgomp.oacc-fortran/host_data-1.f90 at least.

..., but this one now FAILs (ICE) as follows:


[...]/source-gcc/libgomp/testsuite/libgomp.oacc-fortran/host_data-1.f90:11:0: 
internal compiler error: in scan_omp_target, at omp-low.c:3218
0xa33e80 scan_omp_target
[...]/source-gcc/gcc/omp-low.c:3218
0xa33e80 scan_omp_1_stmt
[...]/source-gcc/gcc/omp-low.c:3980
0x8e4e7e walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:555
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0x8e4f62 walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:583
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0x8e4ff2 walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:619
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0xa02479 scan_omp
[...]/source-gcc/gcc/omp-low.c:4024
0xa32ea5 scan_omp_target
[...]/source-gcc/gcc/omp-low.c:3204
0xa32ea5 scan_omp_1_stmt
[...]/source-gcc/gcc/omp-low.c:3980
0x8e4e7e walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:555
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0x8e4ff2 walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:619
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0x8e4f62 walk_gimple_stmt(gimple_stmt_iterator*, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:583
0x8e50b8 walk_gimple_seq_mod(gimple**, tree_node* 
(*)(gimple_stmt_iterator*, bool*, walk_stmt_info*), tree_node* (*)(tree_node**, 
int*, void*), walk_stmt_info*)
[...]/source-gcc/gcc/gimple-walk.c:51
0xa02479 scan_omp
[...]/source-gcc/gcc/omp-low.c:4024
0xa3f35a execute_lower_omp
[...]/source-gcc/gcc/omp-low.c:16735
0xa3f35a execute
[...]/source-gcc/gcc/omp-low.c:16782

Maybe that's due to the gcc/gimplify.c:gimplify_scan_omp_clauses issue
mentioned in
,
or maybe something else?  (XFAILed for now.)

(For avoidance of doubt, the merge does not include my "Some OpenACC
host_data cleanup" commit, trunk r231184, which w

Fortran OpenACC host_data construct ICE (was: [gomp4] Re: [OpenACC 0/7] host_data construct)

[Thomas Schwinge]

Hi!

On Wed, 2 Dec 2015 23:13:58 +0100, I wrote:
> On Wed, 2 Dec 2015 16:58:45 +0100, I wrote:
> > Cesar and Jim copied, for help with Fortran and generally testsuite
> > things.

(Just in case you happen to have any ideas.)

> > On Mon, 30 Nov 2015 19:30:34 +, Julian Brown  
> > wrote:
> > > [patch]
> > 
> > First, thanks!
> 
> Aside from a number of formatting/re-ordering changes, the front end
> changes were basically still the same, but otherwise (middle end,
> libgomp) the patch as committed to trunk in r231118 was quite (totally?)
> ;-) different from the code we had on gomp-4_0-branch, so I had to spend
> some time on merging, cleaning things up.

> > Your submission/commit didn't have any execution tests for OpenACC
> > host_data in Fortran.  On gomp-4_0-branch, there is
> > libgomp/testsuite/libgomp.oacc-fortran/host_data-1.f90 at least.
> 
> ..., but this one now FAILs (ICE) as follows:
> 
> 
> [...]/source-gcc/libgomp/testsuite/libgomp.oacc-fortran/host_data-1.f90:11:0: 
> internal compiler error: in scan_omp_target, at omp-low.c:3218
> 0xa33e80 scan_omp_target
> [...]/source-gcc/gcc/omp-low.c:3218
> [...]

Filed .

> Maybe that's due to the gcc/gimplify.c:gimplify_scan_omp_clauses issue
> mentioned in
> ,
> or maybe something else?  (XFAILed for now.)

The following patch does not resolve the problem -- but we'll still want
something like that, I suppose?

--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -6544,18 +6544,20 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
*pre_p,
 the Fortran FE is updated to OpenMP 4.5.  */
   ctx->target_map_scalars_firstprivate = true;
 }
-  if (!lang_GNU_Fortran ())
-switch (code)
-  {
-  case OMP_TARGET:
-  case OMP_TARGET_DATA:
-  case OMP_TARGET_ENTER_DATA:
-  case OMP_TARGET_EXIT_DATA:
-  case OACC_HOST_DATA:
-   ctx->target_firstprivatize_array_bases = true;
-  default:
+  switch (code)
+{
+case OMP_TARGET:
+case OMP_TARGET_DATA:
+case OMP_TARGET_ENTER_DATA:
+case OMP_TARGET_EXIT_DATA:
+  if (lang_GNU_Fortran ())
break;
-  }
+  /* FALLTHRU */
+case OACC_HOST_DATA:
+  ctx->target_firstprivatize_array_bases = true;
+default:
+  break;
+}
 
   while ((c = *list_p) != NULL)
 {


Grüße
 Thomas


signature.asc
Description: PGP signature