Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
Hi Julian,
On 20.12.23 22:29, Julian Brown wrote:
Thanks for review! Here's a new version of the patch which hopefully
addresses this round of comments.
Thanks for the patch. LGTM now.
Tobias
On Tue, 19 Dec 2023 16:41:54 +0100
Tobias Burnus wrote:
On 16.12.23 14:25, Julian Brown wrote:
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups
(omp_mapping_group *inlist) ard_tail = >next;
break;
+ case GOMP_MAP_PRESENT_ALLOC:
+ *pa_tail = w;
+ w->next = NULL;
+ pa_tail = >next;
+ break;
+
+ case GOMP_MAP_PRESENT_FROM:
+ case GOMP_MAP_PRESENT_TO:
+ case GOMP_MAP_PRESENT_TOFROM:
+ *ptf_tail = w;
+ w->next = NULL;
+ ptf_tail = >next;
+ break;
+
First, I note that GOMP_MAP_PRESENT_ALLOC and
GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the
variable is not present, error termination will happen - otherwise, if
present, no data movement will happen. Hence, they will be changed to
GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses.
That's also the reason that the old code handled all of them
identical.
However, besides a plain 'present', there is also 'always' +
'present'. Those are different as after a normal 'present' check
(abort if not present), the data copying will happen:
GOMP_MAP_ALWAYS_PRESENT_TO, GOMP_MAP_ALWAYS_PRESENT_FROM,
GOMP_MAP_ALWAYS_PRESENT_TOFROM.
(Note that: always + present + alloc = GOMP_MAP_PRESENT_ALLOC (w/o
'always') as already done in the FE.)
Thus, all 'case' from your patch should go to a single group (possibly
adding a comment about it). The question is what to do with the
'present,always' case. I think leaving them under 'default:' is fine,
but I might have missed something.
I've made this change (i.e.: grouping all "GOMP_MAP_PRESENT_*" nodes
together), and in fact that restores the dump output for the
gfortran.dg/gomp/map-12.f90 that needed to be adjusted for the previous
version of the patch (so that hunk has now disappeared).
default:
*tf_tail = w;
w->next = NULL;
@@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups
(omp_mapping_group *inlist)
* * *
@@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p,
gimple_seq *pre_p, break;
}
- if (code == OMP_TARGET
- || code == OMP_TARGET_DATA
- || code == OMP_TARGET_ENTER_DATA
- || code == OMP_TARGET_EXIT_DATA)
-{
- vec *groups;
- groups = omp_gather_mapping_groups (list_p);
- if (groups)
- {
- hash_map
*grpmap;
- grpmap = omp_index_mapping_groups (groups);
+ vec *groups = omp_gather_mapping_groups
(list_p);
+ hash_map *grpmap =
NULL;
+ unsigned grpnum = 0;
+ tree *grp_start_p = NULL, grp_end = NULL_TREE;
...
- else if (region_type & ORT_ACC)
-{
I wonder whether you should not better call
'omp_gather_mapping_groups' only for the 'code == OMP_TARGET...' and
for ORT_ACC (or some subset of OACC *), given that this function is
also called bygimplify_omp_parallel, gimplify_omp_task,
gimplify_omp_for, ...
This avoids some memory allocation and list_p walking, i.e. it is not
too bad - but also not really needed for task, parallel, for, ...
I've made that change -- OpenACC uses OMP_CLAUSE_MAP in quite a wide
range of directives, but the new version of the patch lists them
individually anyway, rather than using a catch-all for ORT_ACC regions.
That seems OK, I think.
@@ -14008,26 +13926,73 @@ gimplify_adjust_omp_clauses (gimple_seq
*pre_p, gimple_seq body, tree *list_p, default:
break;
}
- if (code == OMP_TARGET_EXIT_DATA
- && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER)
+ switch (code)
{
+ case OMP_TARGET:
+ break;
+ case OACC_DATA:
+ if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
+ break;
+ goto check_firstprivate;
+ case OACC_ENTER_DATA:
+ case OACC_EXIT_DATA:
+ case OMP_TARGET_DATA:
+ case OMP_TARGET_ENTER_DATA:
+ case OMP_TARGET_EXIT_DATA:
+ case OACC_HOST_DATA:
+ check_firstprivate:
+ if (OMP_CLAUSE_MAP_KIND (c) ==
GOMP_MAP_FIRSTPRIVATE_POINTER
I think it looks nicer if the OACC_HOST is before OMP_* such that all
OACC_* are together. (In the old code, oacc_enter/exit was treated
differently than OMP_* and OACC_HOST_DATA; your order is a leftover
from that code movement/change.)
I've fixed this bit -- which actually doesn't need the goto any more
either, so that's now a fallthrough instead.
+ flags = GOVD_MAP | GOVD_EXPLICIT;
+ if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TO
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TOFROM)
+ flags |= GOVD_MAP_ALWAYS_TO;
I know that the code has only been moved, but I wonder whether that
should also include GOMP_MAP_ALWAYS_PRESENT_{TO,TOFROM} as condition.
I've added it (caveat: without any tests).
Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
Hi Tobias,
Thanks for review! Here's a new version of the patch which hopefully
addresses this round of comments.
On Tue, 19 Dec 2023 16:41:54 +0100
Tobias Burnus wrote:
> On 16.12.23 14:25, Julian Brown wrote:
> > --- a/gcc/gimplify.cc
> > +++ b/gcc/gimplify.cc
> > @@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups
> > (omp_mapping_group *inlist) ard_tail = >next;
> > break;
> >
> > + case GOMP_MAP_PRESENT_ALLOC:
> > + *pa_tail = w;
> > + w->next = NULL;
> > + pa_tail = >next;
> > + break;
> > +
> > + case GOMP_MAP_PRESENT_FROM:
> > + case GOMP_MAP_PRESENT_TO:
> > + case GOMP_MAP_PRESENT_TOFROM:
> > + *ptf_tail = w;
> > + w->next = NULL;
> > + ptf_tail = >next;
> > + break;
> > +
>
> First, I note that GOMP_MAP_PRESENT_ALLOC and
> GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the
> variable is not present, error termination will happen - otherwise, if
> present, no data movement will happen. Hence, they will be changed to
> GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses.
>
> That's also the reason that the old code handled all of them
> identical.
>
> However, besides a plain 'present', there is also 'always' +
> 'present'. Those are different as after a normal 'present' check
> (abort if not present), the data copying will happen:
> GOMP_MAP_ALWAYS_PRESENT_TO, GOMP_MAP_ALWAYS_PRESENT_FROM,
> GOMP_MAP_ALWAYS_PRESENT_TOFROM.
>
> (Note that: always + present + alloc = GOMP_MAP_PRESENT_ALLOC (w/o
> 'always') as already done in the FE.)
>
> Thus, all 'case' from your patch should go to a single group (possibly
> adding a comment about it). The question is what to do with the
> 'present,always' case. I think leaving them under 'default:' is fine,
> but I might have missed something.
I've made this change (i.e.: grouping all "GOMP_MAP_PRESENT_*" nodes
together), and in fact that restores the dump output for the
gfortran.dg/gomp/map-12.f90 that needed to be adjusted for the previous
version of the patch (so that hunk has now disappeared).
> > default:
> > *tf_tail = w;
> > w->next = NULL;
> > @@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups
> > (omp_mapping_group *inlist)
> * * *
> > @@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p,
> > gimple_seq *pre_p, break;
> > }
> >
> > - if (code == OMP_TARGET
> > - || code == OMP_TARGET_DATA
> > - || code == OMP_TARGET_ENTER_DATA
> > - || code == OMP_TARGET_EXIT_DATA)
> > -{
> > - vec *groups;
> > - groups = omp_gather_mapping_groups (list_p);
> > - if (groups)
> > - {
> > - hash_map
> > *grpmap;
> > - grpmap = omp_index_mapping_groups (groups);
> > + vec *groups = omp_gather_mapping_groups
> > (list_p);
> > + hash_map *grpmap =
> > NULL;
> > + unsigned grpnum = 0;
> > + tree *grp_start_p = NULL, grp_end = NULL_TREE;
>
> ...
>
> > - else if (region_type & ORT_ACC)
> > -{
> I wonder whether you should not better call
> 'omp_gather_mapping_groups' only for the 'code == OMP_TARGET...' and
> for ORT_ACC (or some subset of OACC *), given that this function is
> also called bygimplify_omp_parallel, gimplify_omp_task,
> gimplify_omp_for, ...
>
> This avoids some memory allocation and list_p walking, i.e. it is not
> too bad - but also not really needed for task, parallel, for, ...
I've made that change -- OpenACC uses OMP_CLAUSE_MAP in quite a wide
range of directives, but the new version of the patch lists them
individually anyway, rather than using a catch-all for ORT_ACC regions.
That seems OK, I think.
> > @@ -14008,26 +13926,73 @@ gimplify_adjust_omp_clauses (gimple_seq
> > *pre_p, gimple_seq body, tree *list_p, default:
> > break;
> > }
> > - if (code == OMP_TARGET_EXIT_DATA
> > - && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_POINTER)
> > + switch (code)
> > {
> > + case OMP_TARGET:
> > + break;
> > + case OACC_DATA:
> > + if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
> > + break;
> > + goto check_firstprivate;
> > + case OACC_ENTER_DATA:
> > + case OACC_EXIT_DATA:
> > + case OMP_TARGET_DATA:
> > + case OMP_TARGET_ENTER_DATA:
> > + case OMP_TARGET_EXIT_DATA:
> > + case OACC_HOST_DATA:
> > + check_firstprivate:
> > + if (OMP_CLAUSE_MAP_KIND (c) ==
> > GOMP_MAP_FIRSTPRIVATE_POINTER
>
> I think it looks nicer if the OACC_HOST is before OMP_* such that all
> OACC_* are together. (In the old code, oacc_enter/exit was treated
> differently than OMP_* and OACC_HOST_DATA; your order is a leftover
> from that code movement/change.)
I've fixed this bit -- which actually doesn't need the goto any more
either, so that's now a fallthrough instead.
> > + flags = GOVD_MAP | GOVD_EXPLICIT;
> > + if (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_TO
> > +
Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
Hi Julian,
On 16.12.23 14:25, Julian Brown wrote:
OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
This patch has been separated out from the C++ "declare mapper"
support patch. It contains just the gimplify.cc rearrangement
work, mostly moving gimplification from gimplify_scan_omp_clauses
to gimplify_adjust_omp_clauses for map clauses.
The motivation for doing this was that we don't know if we need to
instantiate mappers implicitly until the body of an offload region has
been scanned, i.e. in gimplify_adjust_omp_clauses, but we also need the
un-gimplified form of clauses to sort by base-pointer dependencies after
mapper instantiation has taken place.
The patch also reimplements the "present" clause sorting code to avoid
another sorting pass on mapping nodes.
This version of the patch is based on the version posted for og13, and
additionally incorporates a follow-on fix for DECL_VALUE_EXPR handling
in gimplify_adjust_omp_clauses:
"OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc"
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/63.html
Parts of:
"OpenMP: OpenMP 5.2 semantics for pointers with unmapped target"
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/623351.html
I find the patch hard to digest - and I wouldn't be surprised if I
missed something.
However, I think the patch is OK - except for a few minor observations:
2023-12-16 Julian Brown
gcc/
* gimplify.cc (omp_segregate_mapping_groups): Handle "present"
groups.
(gimplify_scan_omp_clauses): Use mapping group functionality to
iterate through mapping nodes. Remove most gimplification of
OMP_CLAUSE_MAP nodes from here, but still populate ctx->variables
splay tree.
(gimplify_adjust_omp_clauses): Move most gimplification of
OMP_CLAUSE_MAP nodes here.
gcc/testsuite/
* gfortran.dg/gomp/map-12.f90: Adjust scan output.
libgomp/
* testsuite/libgomp.fortran/target-enter-data-6.f90: Remove XFAIL.
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index a6bdceab45d..fa6ddd546f8 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -10107,6 +10114,20 @@ omp_segregate_mapping_groups (omp_mapping_group
*inlist)
ard_tail = >next;
break;
+ case GOMP_MAP_PRESENT_ALLOC:
+ *pa_tail = w;
+ w->next = NULL;
+ pa_tail = >next;
+ break;
+
+ case GOMP_MAP_PRESENT_FROM:
+ case GOMP_MAP_PRESENT_TO:
+ case GOMP_MAP_PRESENT_TOFROM:
+ *ptf_tail = w;
+ w->next = NULL;
+ ptf_tail = >next;
+ break;
+
First, I note that GOMP_MAP_PRESENT_ALLOC and
GOMP_MAP_PRESENT_{FROM,TO,TOFROM} are semantically identical: If the
variable is not present, error termination will happen - otherwise, if
present, no data movement will happen. Hence, they will be changed to
GOMP_MAP_FORCE_PRESENT in gimplify_adjust_omp_clauses.
That's also the reason that the old code handled all of them identical.
However, besides a plain 'present', there is also 'always' + 'present'.
Those are different as after a normal 'present' check (abort if not
present), the data copying will happen: GOMP_MAP_ALWAYS_PRESENT_TO,
GOMP_MAP_ALWAYS_PRESENT_FROM, GOMP_MAP_ALWAYS_PRESENT_TOFROM.
(Note that: always + present + alloc = GOMP_MAP_PRESENT_ALLOC (w/o
'always') as already done in the FE.)
Thus, all 'case' from your patch should go to a single group (possibly
adding a comment about it). The question is what to do with the
'present,always' case. I think leaving them under 'default:' is fine,
but I might have missed something.
default:
*tf_tail = w;
w->next = NULL;
@@ -10118,8 +10139,10 @@ omp_segregate_mapping_groups (omp_mapping_group
*inlist)
* * *
@@ -11922,119 +11945,30 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq
*pre_p,
break;
}
- if (code == OMP_TARGET
- || code == OMP_TARGET_DATA
- || code == OMP_TARGET_ENTER_DATA
- || code == OMP_TARGET_EXIT_DATA)
-{
- vec *groups;
- groups = omp_gather_mapping_groups (list_p);
- if (groups)
- {
- hash_map *grpmap;
- grpmap = omp_index_mapping_groups (groups);
+ vec *groups = omp_gather_mapping_groups (list_p);
+ hash_map *grpmap = NULL;
+ unsigned grpnum = 0;
+ tree *grp_start_p = NULL, grp_end = NULL_TREE;
...
- else if (region_type & ORT_ACC)
-{
I wonder whether you should not better call 'omp_gather_mapping_groups'
only for the 'code == OMP_TARGET...' and for ORT_ACC (or some subset of
OACC *), given that this function is also called
bygimplify_omp_parallel, gimplify_omp_task, gimplify_omp_for, ...
This avoids some memory allocation and list_p walking, i.e. it is not
too bad - but also not really needed for task, parallel, for, ...
@@ -14008,26 +13926,73 @@
[PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc
This patch has been separated out from the C++ "declare mapper"
support patch. It contains just the gimplify.cc rearrangement
work, mostly moving gimplification from gimplify_scan_omp_clauses
to gimplify_adjust_omp_clauses for map clauses.
The motivation for doing this was that we don't know if we need to
instantiate mappers implicitly until the body of an offload region has
been scanned, i.e. in gimplify_adjust_omp_clauses, but we also need the
un-gimplified form of clauses to sort by base-pointer dependencies after
mapper instantiation has taken place.
The patch also reimplements the "present" clause sorting code to avoid
another sorting pass on mapping nodes.
This version of the patch is based on the version posted for og13, and
additionally incorporates a follow-on fix for DECL_VALUE_EXPR handling
in gimplify_adjust_omp_clauses:
"OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc"
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/63.html
Parts of:
"OpenMP: OpenMP 5.2 semantics for pointers with unmapped target"
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/623351.html
2023-08-18 Julian Brown
gcc/
* gimplify.cc (omp_segregate_mapping_groups): Handle "present" groups.
(gimplify_scan_omp_clauses): Use mapping group functionality to
iterate through mapping nodes. Remove most gimplification of
OMP_CLAUSE_MAP nodes from here, but still populate ctx->variables
splay tree.
(gimplify_adjust_omp_clauses): Move most gimplification of
OMP_CLAUSE_MAP nodes here.
gcc/testsuite/
* gfortran.dg/gomp/map-12.f90: Adjust scan output.
---
gcc/gimplify.cc | 667 +-
gcc/testsuite/gfortran.dg/gomp/map-12.f90 | 2 +-
2 files changed, 386 insertions(+), 283 deletions(-)
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index e682583054b0..1e32ad48b844 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -9804,10 +9804,15 @@ omp_tsort_mapping_groups (vec
*groups,
return outlist;
}
-/* Split INLIST into two parts, moving groups corresponding to
- ALLOC/RELEASE/DELETE mappings to one list, and other mappings to another.
- The former list is then appended to the latter. Each sub-list retains the
- order of the original list.
+/* Split INLIST into four parts:
+
+ - "present" to/from groups
+ - "present" alloc groups
+ - other to/from groups
+ - other alloc/release/delete groups
+
+ These sub-lists are then concatenated together to form the final list.
+ Each sub-list retains the order of the original list.
Note that ATTACH nodes are later moved to the end of the list in
gimplify_adjust_omp_clauses, for target regions. */
@@ -9815,7 +9820,9 @@ static omp_mapping_group *
omp_segregate_mapping_groups (omp_mapping_group *inlist)
{
omp_mapping_group *ard_groups = NULL, *tf_groups = NULL;
+ omp_mapping_group *pa_groups = NULL, *ptf_groups = NULL;
omp_mapping_group **ard_tail = _groups, **tf_tail = _groups;
+ omp_mapping_group **pa_tail = _groups, **ptf_tail = _groups;
for (omp_mapping_group *w = inlist; w;)
{
@@ -9834,6 +9841,20 @@ omp_segregate_mapping_groups (omp_mapping_group *inlist)
ard_tail = >next;
break;
+ case GOMP_MAP_PRESENT_ALLOC:
+ *pa_tail = w;
+ w->next = NULL;
+ pa_tail = >next;
+ break;
+
+ case GOMP_MAP_PRESENT_FROM:
+ case GOMP_MAP_PRESENT_TO:
+ case GOMP_MAP_PRESENT_TOFROM:
+ *ptf_tail = w;
+ w->next = NULL;
+ ptf_tail = >next;
+ break;
+
default:
*tf_tail = w;
w->next = NULL;
@@ -9845,8 +9866,10 @@ omp_segregate_mapping_groups (omp_mapping_group *inlist)
/* Now splice the lists together... */
*tf_tail = ard_groups;
+ *pa_tail = tf_groups;
+ *ptf_tail = pa_groups;
- return tf_groups;
+ return ptf_groups;
}
/* Given a list LIST_P containing groups of mappings given by GROUPS, reorder
@@ -11698,119 +11721,30 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq
*pre_p,
break;
}
- if (code == OMP_TARGET
- || code == OMP_TARGET_DATA
- || code == OMP_TARGET_ENTER_DATA
- || code == OMP_TARGET_EXIT_DATA)
-{
- vec *groups;
- groups = omp_gather_mapping_groups (list_p);
- if (groups)
- {
- hash_map *grpmap;
- grpmap = omp_index_mapping_groups (groups);
+ vec *groups = omp_gather_mapping_groups (list_p);
+ hash_map *grpmap = NULL;
+ unsigned grpnum = 0;
+ tree *grp_start_p = NULL, grp_end = NULL_TREE;
- omp_resolve_clause_dependencies (code, groups, grpmap);
- omp_build_struct_sibling_lists (code, region_type, groups, ,
- list_p);
-
- omp_mapping_group *outlist = NULL;
- bool enter_exit = (code == OMP_TARGET_ENTER_DATA
-|| code == OMP_TARGET_EXIT_DATA);
-
- /* Topological
