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 = &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 *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-tested with offloading to NVPTX. OK now? Thanks, Julian