[PATCH, OpenACC] Rework computation of default OpenACC mapping clauses

[PATCH, OpenACC] Rework computation of default OpenACC mapping clauses

[Gergö Barany]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

This patch unifies and simplifies the handling of OpenACC default 
mapping clauses for parallel, serial, and kernels regions.

OK for openacc-gcc-8-branch?

Thanks,
Gergö


     gcc/
     * gimplify.c (oacc_default_clause): Refactor and unify computation of
     default mapping clauses.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Rework-computation-of-default-OpenACC-mapping-clause.patch --]
[-- Type: text/x-patch; name="0001-Rework-computation-of-default-OpenACC-mapping-clause.patch", Size: 4172 bytes --]

From 32a38daf2084bb266aa3a0c61c9176098d2d4bdb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerg=C3=B6=20Barany?= <gergo@codesourcery.com>
Date: Mon, 21 Jan 2019 03:01:02 -0800
Subject: [PATCH] Rework computation of default OpenACC mapping clauses

    gcc/
    * gimplify.c (oacc_default_clause): Refactor and unify computation of
    default mapping clauses.
---
 gcc/ChangeLog.openacc |  5 ++++
 gcc/gimplify.c        | 75 +++++++++++++++++++++++++--------------------------
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/gcc/ChangeLog.openacc b/gcc/ChangeLog.openacc
index 22cdb5b..932fb37 100644
--- a/gcc/ChangeLog.openacc
+++ b/gcc/ChangeLog.openacc
@@ -1,3 +1,8 @@
+2019-01-24  Gergö Barany  <gergo@codesourcery.com>
+
+	* gimplify.c (oacc_default_clause): Refactor and unify computation of
+	default mapping clauses.
+
 2019-01-09  Julian Brown  <julian@codesourcery.com>
 
 	* doc/invoke.texi: Update mention of OpenACC version to 2.6.
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index a60e395..a6a4d2a 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -7191,58 +7191,55 @@ oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags)
       flags |= GOVD_MAP_TO_ONLY;
     }
 
+  unsigned private_mapping_flag = 0;
+  unsigned default_scalar_flags = 0;
+  /* Aggregates default to 'present_or_copy', or 'present'.  */
+  unsigned aggregate_flags
+    = (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT
+        ? GOVD_MAP
+        : GOVD_MAP | GOVD_MAP_FORCE_PRESENT);
+
   switch (ctx->region_type)
     {
     case ORT_ACC_KERNELS:
       rkind = "kernels";
-
-      if (is_private)
-	flags |= GOVD_MAP;
-      else if (AGGREGATE_TYPE_P (type))
-	{
-	  /* Aggregates default to 'present_or_copy', or 'present'.  */
-	  if (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT)
-	    flags |= GOVD_MAP;
-	  else
-	    flags |= GOVD_MAP | GOVD_MAP_FORCE_PRESENT;
-	}
-      else
-	/* Scalars default to 'copy'.  */
-	flags |= GOVD_MAP | GOVD_MAP_FORCE;
-
+      /* Scalars default to 'copy'.  */
+      default_scalar_flags = GOVD_MAP | GOVD_MAP_FORCE;
+      /* There are no private mappings on kernels regions.  */
+      gcc_assert (!is_private);
       break;
-
     case ORT_ACC_PARALLEL:
+      rkind = "parallel";
+      /* Scalars default to 'firstprivate'.  */
+      default_scalar_flags = GOVD_FIRSTPRIVATE;
+      private_mapping_flag = GOVD_FIRSTPRIVATE;
+      break;
     case ORT_ACC_SERIAL:
-      rkind = ctx->region_type == ORT_ACC_PARALLEL ? "parallel" : "serial";
-
-      if (TREE_CODE (type) == REFERENCE_TYPE
-	  && TREE_CODE (TREE_TYPE (type)) == POINTER_TYPE)
-	flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
-      else if (!lang_GNU_Fortran () && TREE_CODE (type) == POINTER_TYPE)
-	flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
-      else if (is_private)
-	flags |= GOVD_FIRSTPRIVATE;
-      else if (on_device || declared)
-	flags |= GOVD_MAP;
-      else if (AGGREGATE_TYPE_P (type))
-	{
-	  /* Aggregates default to 'present_or_copy', or 'present'.  */
-	  if (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT)
-	    flags |= GOVD_MAP;
-	  else
-	    flags |= GOVD_MAP | GOVD_MAP_FORCE_PRESENT;
-	}
-      else
-	/* Scalars default to 'firstprivate'.  */
-	flags |= GOVD_FIRSTPRIVATE;
-
+      rkind = "serial";
+      /* Scalars default to 'firstprivate'.  */
+      default_scalar_flags = GOVD_FIRSTPRIVATE;
+      private_mapping_flag = GOVD_FIRSTPRIVATE;
       break;
 
     default:
       gcc_unreachable ();
     }
 
+  if (TREE_CODE (type) == REFERENCE_TYPE
+      && TREE_CODE (TREE_TYPE (type)) == POINTER_TYPE)
+    flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
+  else if (!lang_GNU_Fortran () && TREE_CODE (type) == POINTER_TYPE)
+    flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
+  else if (is_private)
+    flags |= private_mapping_flag;
+  else if (on_device || declared)
+    flags |= GOVD_MAP;
+  else if (AGGREGATE_TYPE_P (type))
+    flags |= aggregate_flags;
+  else
+    /* This is a scalar getting the default mapping.  */
+    flags |= default_scalar_flags;
+
   if (DECL_ARTIFICIAL (decl))
     ; /* We can get compiler-generated decls, and should not complain
 	 about them.  */
-- 
2.8.1

Re: [PATCH, OpenACC] Rework computation of default OpenACC mapping clauses

[Thomas Schwinge]

Hi Gergő!

On Fri, 25 Jan 2019 15:09:49 +0100, Gergö Barany <gergo_barany@mentor.com> wrote:
> This patch unifies and simplifies the handling of OpenACC default 
> mapping clauses for parallel, serial, and kernels regions.

Thanks.  This answers/resolves at least some of the questions that I
asked (not you) many months ago, about why 'kernels' and 'parallel'
constructs are being treated differently here.

I'll be good to eventually add proper testing so that we verify (at
"gimple" tree-dump level?) that the expected mappings are set up under
all the different conditions, but your patch certainly is an improvement
already, so:

> OK for openacc-gcc-8-branch?

Yes.


A few comments (for later):

> From 32a38daf2084bb266aa3a0c61c9176098d2d4bdb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Gerg=C3=B6=20Barany?= <gergo@codesourcery.com>
> Date: Mon, 21 Jan 2019 03:01:02 -0800
> Subject: [PATCH] Rework computation of default OpenACC mapping clauses
> 
>     gcc/
>     * gimplify.c (oacc_default_clause): Refactor and unify computation of
>     default mapping clauses.
> ---
>  gcc/ChangeLog.openacc |  5 ++++
>  gcc/gimplify.c        | 75 +++++++++++++++++++++++++--------------------------
>  2 files changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/gcc/ChangeLog.openacc b/gcc/ChangeLog.openacc
> index 22cdb5b..932fb37 100644
> --- a/gcc/ChangeLog.openacc
> +++ b/gcc/ChangeLog.openacc
> @@ -1,3 +1,8 @@
> +2019-01-24  Gergö Barany  <gergo@codesourcery.com>
> +
> +	* gimplify.c (oacc_default_clause): Refactor and unify computation of
> +	default mapping clauses.
> +
>  2019-01-09  Julian Brown  <julian@codesourcery.com>
>  
>  	* doc/invoke.texi: Update mention of OpenACC version to 2.6.
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index a60e395..a6a4d2a 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -7191,58 +7191,55 @@ oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags)
>        flags |= GOVD_MAP_TO_ONLY;
>      }
>  
> +  unsigned private_mapping_flag = 0;
> +  unsigned default_scalar_flags = 0;

It might make sense to initialize these to some "invalid" values, so that
we can at some later point detect when these are being used, when they
shouldn't.

> +  /* Aggregates default to 'present_or_copy', or 'present'.  */
> +  unsigned aggregate_flags
> +    = (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT
> +        ? GOVD_MAP
> +        : GOVD_MAP | GOVD_MAP_FORCE_PRESENT);
> +
>    switch (ctx->region_type)
>      {
>      case ORT_ACC_KERNELS:
>        rkind = "kernels";
> -
> -      if (is_private)
> -	flags |= GOVD_MAP;
> -      else if (AGGREGATE_TYPE_P (type))
> -	{
> -	  /* Aggregates default to 'present_or_copy', or 'present'.  */
> -	  if (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT)
> -	    flags |= GOVD_MAP;
> -	  else
> -	    flags |= GOVD_MAP | GOVD_MAP_FORCE_PRESENT;
> -	}
> -      else
> -	/* Scalars default to 'copy'.  */
> -	flags |= GOVD_MAP | GOVD_MAP_FORCE;
> -
> +      /* Scalars default to 'copy'.  */
> +      default_scalar_flags = GOVD_MAP | GOVD_MAP_FORCE;
> +      /* There are no private mappings on kernels regions.  */
> +      gcc_assert (!is_private);
>        break;

Ah, is this literally the 'private' clause?  (Or something else -- I have
not yet looked up what exactly "is_private =
lang_hooks.decls.omp_disregard_value_expr (decl, false)" is doing "if
(RECORD_OR_UNION_TYPE_P (type))".)

> -
>      case ORT_ACC_PARALLEL:
> +      rkind = "parallel";
> +      /* Scalars default to 'firstprivate'.  */
> +      default_scalar_flags = GOVD_FIRSTPRIVATE;
> +      private_mapping_flag = GOVD_FIRSTPRIVATE;
> +      break;
>      case ORT_ACC_SERIAL:
> -      rkind = ctx->region_type == ORT_ACC_PARALLEL ? "parallel" : "serial";
> -
> -      if (TREE_CODE (type) == REFERENCE_TYPE
> -	  && TREE_CODE (TREE_TYPE (type)) == POINTER_TYPE)
> -	flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
> -      else if (!lang_GNU_Fortran () && TREE_CODE (type) == POINTER_TYPE)
> -	flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
> -      else if (is_private)
> -	flags |= GOVD_FIRSTPRIVATE;
> -      else if (on_device || declared)
> -	flags |= GOVD_MAP;
> -      else if (AGGREGATE_TYPE_P (type))
> -	{
> -	  /* Aggregates default to 'present_or_copy', or 'present'.  */
> -	  if (ctx->default_kind != OMP_CLAUSE_DEFAULT_PRESENT)
> -	    flags |= GOVD_MAP;
> -	  else
> -	    flags |= GOVD_MAP | GOVD_MAP_FORCE_PRESENT;
> -	}
> -      else
> -	/* Scalars default to 'firstprivate'.  */
> -	flags |= GOVD_FIRSTPRIVATE;
> -
> +      rkind = "serial";
> +      /* Scalars default to 'firstprivate'.  */
> +      default_scalar_flags = GOVD_FIRSTPRIVATE;
> +      private_mapping_flag = GOVD_FIRSTPRIVATE;
>        break;
>  
>      default:
>        gcc_unreachable ();
>      }
>  
> +  if (TREE_CODE (type) == REFERENCE_TYPE
> +      && TREE_CODE (TREE_TYPE (type)) == POINTER_TYPE)
> +    flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
> +  else if (!lang_GNU_Fortran () && TREE_CODE (type) == POINTER_TYPE)
> +    flags |= GOVD_MAP | GOVD_MAP_0LEN_ARRAY;
> +  else if (is_private)
> +    flags |= private_mapping_flag;
> +  else if (on_device || declared)
> +    flags |= GOVD_MAP;
> +  else if (AGGREGATE_TYPE_P (type))
> +    flags |= aggregate_flags;
> +  else

Should probably have here some explicit "else if ([something])"
condition, and...

> +    /* This is a scalar getting the default mapping.  */
> +    flags |= default_scalar_flags;

... here add an "else gcc_unreachable ()" instead of the "catch-all
else", so that we'll catch any remaining logic errors, instead of
silently mapping as scalars.

> +
>    if (DECL_ARTIFICIAL (decl))
>      ; /* We can get compiler-generated decls, and should not complain
>  	 about them.  */


Grüße
 Thomas

Re: [PATCH, OpenACC] Rework computation of default OpenACC mapping clauses

[Gergö Barany]

On 28/01/2019 18:00, Thomas Schwinge wrote:
> On Fri, 25 Jan 2019 15:09:49 +0100, Gergö Barany <gergo_barany@mentor.com> wrote:
>> OK for openacc-gcc-8-branch?
> 
> Yes.

Thanks, committed along with the other patches I posted at the same time 
(Rework OpenACC Fortran DO loop initialization, Remove spurious OpenACC 
error on combining "auto" with gang/worker/vector), and with 
Reviewed-by: notes added to the commit messages.


Thanks,
Gergö