Re: [PATCH v7 5/5] OpenMP/OpenACC: Reorganise OMP map clause handling in gimplify.cc

[Tobias Burnus <tobias@codesourcery.com>]

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<julian@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