Hi Cesar! On Wed, 5 Apr 2017 13:37:30 -0700, Cesar Philippidis <ce...@codesourcery.com> wrote: > On 04/05/2017 01:22 PM, Thomas Schwinge wrote: > > >> --- a/gcc/gimplify.c > >> +++ b/gcc/gimplify.c > >> @@ -6102,14 +6102,19 @@ oacc_default_clause (struct gimplify_omp_ctx *ctx, > >> tree decl, unsigned flags) > >> { > >> const char *rkind; > >> bool on_device = false; > >> + bool is_private = false; > > > > So the intention here is that by default everything stays the same as > > before; "is_private == false". This property is satisfied in the > > following code. > > Yes. > > >> tree type = TREE_TYPE (decl); > >> > >> if (lang_hooks.decls.omp_privatize_by_reference (decl)) > >> type = TREE_TYPE (type); > >> > >> + if (RECORD_OR_UNION_TYPE_P (type)) > >> + is_private = lang_hooks.decls.omp_disregard_value_expr (decl, false); > >> + > >> if ((ctx->region_type & (ORT_ACC_PARALLEL | ORT_ACC_KERNELS)) != 0 > >> && is_global_var (decl) > >> - && device_resident_p (decl)) > >> + && device_resident_p (decl) > >> + && !is_private) > >> { > >> on_device = true; > >> flags |= GOVD_MAP_TO_ONLY; > > | } > > > > For "is_private == true" we will not possibly enter this block.
So, is this an invalid scenario (and should thus get an "assert" or "gcc_unreachable"), or is it correct, if "private", to not set "on_device" and "GOVD_MAP_TO_ONLY" for "device_resident_p" DECLs? > > | [ORT_ACC_KERNELS] > >> /* Scalars are default 'copy' under kernels, non-scalars are default > >> 'present_or_copy'. */ > >> flags |= GOVD_MAP; > >> - if (!AGGREGATE_TYPE_P (type)) > >> + if (!AGGREGATE_TYPE_P (type) && !is_private) > >> flags |= GOVD_MAP_FORCE; > > > > For "is_private == true" we will not possibly enter this block, which > > means in this case we will map both scalars and aggregates as > > "present_or_copy". > > Yes. Inside kernels regions, everything is pcopy, unless it's private. But I'm saying/understanding the code so that inside kernels regions, "private" stuff is "present_or_copy". Is that correct or not? > Some private variables include, e.g., fortran array descriptors. (I'd prefer if we had tree-dump scanning tests for such things.) > >> case ORT_ACC_PARALLEL: > >> { > >> - if (on_device || AGGREGATE_TYPE_P (type)) > >> + if (!is_private && (on_device || AGGREGATE_TYPE_P (type))) > >> /* Aggregates default to 'present_or_copy'. */ > >> flags |= GOVD_MAP; > >> else > > | /* Scalars default to 'firstprivate'. */ > > | flags |= GOVD_FIRSTPRIVATE; > > > > For "is_private == true" we will not possibly enter the "if" block, so we > > will always enter the "else" block, which means in this case we will map > > both scalars and aggregates as "firstprivate". > > > > Is that all correct? > > Yes. Is there something wrong with that behavior Again: I'm confused as to why inside kernels regions, "private" stuff is mapped "present_or_copy", but inside parallel regions, it's "firstprivate". > or is it just unclear? In gomp-4_0-branch r246762, I committed the following patch, to make better apparent the current behavior: commit a87af0655eb2f803c330d807cdca698ab597b44e Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Fri Apr 7 14:55:56 2017 +0000 Clarify gcc/gimplify.c:oacc_default_clause gcc/ * gimplify.c (oacc_default_clause): Clarify. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@246762 138bc75d-0d04-0410-961f-82ee72b054a4 --- gcc/ChangeLog.gomp | 4 ++++ gcc/gimplify.c | 44 ++++++++++++++++++++++++++------------------ 2 files changed, 30 insertions(+), 18 deletions(-) diff --git gcc/ChangeLog.gomp gcc/ChangeLog.gomp index effcc05..811e190 100644 --- gcc/ChangeLog.gomp +++ gcc/ChangeLog.gomp @@ -1,3 +1,7 @@ +2017-04-07 Thomas Schwinge <tho...@codesourcery.com> + + * gimplify.c (oacc_default_clause): Clarify. + 2017-04-05 Cesar Philippidis <ce...@codesourcery.com> * omp-low.c (scan_sharing_clauses): Update handling of OpenACC declare diff --git gcc/gimplify.c gcc/gimplify.c index 604a6cb..f2bb814 100644 --- gcc/gimplify.c +++ gcc/gimplify.c @@ -6129,30 +6129,38 @@ oacc_default_clause (struct gimplify_omp_ctx *ctx, tree decl, unsigned flags) switch (ctx->region_type) { - default: - gcc_unreachable (); - case ORT_ACC_KERNELS: - /* Scalars are default 'copy' under kernels, non-scalars are default - 'present_or_copy'. */ - flags |= GOVD_MAP; - if (!AGGREGATE_TYPE_P (type) && !is_private) - flags |= GOVD_MAP_FORCE; - rkind = "kernels"; + + if (is_private) + flags |= GOVD_MAP; + else if (AGGREGATE_TYPE_P (type)) + /* Aggregates default to 'present_or_copy'. */ + flags |= GOVD_MAP; + else + /* Scalars default to 'copy'. */ + flags |= GOVD_MAP | GOVD_MAP_FORCE; + break; case ORT_ACC_PARALLEL: - { - if (!is_private && (on_device || AGGREGATE_TYPE_P (type) || declared)) - /* Aggregates default to 'present_or_copy'. */ - flags |= GOVD_MAP; - else - /* Scalars default to 'firstprivate'. */ - flags |= GOVD_FIRSTPRIVATE; - rkind = "parallel"; - } + rkind = "parallel"; + + 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'. */ + flags |= GOVD_MAP; + else + /* Scalars default to 'firstprivate'. */ + flags |= GOVD_FIRSTPRIVATE; + break; + + default: + gcc_unreachable (); } if (DECL_ARTIFICIAL (decl)) Grüße Thomas