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/622223.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<jul...@codesourcery.com>
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 = &w->next;
break;
+ case GOMP_MAP_PRESENT_ALLOC:
+ *pa_tail = w;
+ w->next = NULL;
+ pa_tail = &w->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 = &w->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<omp_mapping_group> *groups;
- groups = omp_gather_mapping_groups (list_p);
- if (groups)
- {
- hash_map<tree_operand_hash_no_se, omp_mapping_group *> *grpmap;
- grpmap = omp_index_mapping_groups (groups);
+ vec<omp_mapping_group> *groups = omp_gather_mapping_groups (list_p);
+ hash_map<tree_operand_hash_no_se, omp_mapping_group *> *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 @@ 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.)
* * *
+ 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.
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
