Message ID | 5b5e33d1-c25e-47a9-bfbb-dbbeec865640@mentor.com |
---|---|
State | New |
Headers | show |
Series | [OpenACC] Rework computation of default OpenACC mapping clauses | expand |
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
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ö
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