Hi Gergő!
On Fri, 25 Jan 2019 15:09:49 +0100, Gergö Barany <gergo_bar...@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?= <ge...@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 <ge...@codesourcery.com>
> +
> + * gimplify.c (oacc_default_clause): Refactor and unify computation of
> + default mapping clauses.
> +
> 2019-01-09 Julian Brown <jul...@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
