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 <tob...@codesourcery.com> 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 = &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.
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<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, ...
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-tested with offloading to NVPTX. OK now?
-----------------
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
