On Tue, Sep 01, 2020 at 09:16:23PM +0800, Chung-Lin Tang wrote:
> this patch set implements parts of the target mapping changes introduced
> in OpenMP 5.0, mainly the attachment requirements for pointer-based
> list items, and the clause ordering.
>
> The first patch here are the C/C++ front-end changes.
>
> The entire set of changes has been tested for without regressions for
> the compiler and libgomp. Hope this is ready to commit to master.
Sorry for the delay in patch review and thanks for the standard citations,
that really helps.
> gcc/c-family/
> * c-common.h (c_omp_adjust_clauses): New declaration.
> * c-omp.c (c_omp_adjust_clauses): New function.
Besides the naming, I wonder why is it done in a separate function and so
early, can't what the function does be done either in
{,c_}finish_omp_clauses (provided we'd pass separate ORT_OMP vs.
ORT_OMP_TARGET to it to determine if it is target region vs. anything else),
or perhaps even better during gimplification (gimplify_scan_omp_clauses)?
> gcc/c/
> * c-parser.c (c_parser_omp_target_data): Add use of
> new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as
> handled map clause kind.
> (c_parser_omp_target_enter_data): Likewise.
> (c_parser_omp_target_exit_data): Likewise.
> (c_parser_omp_target): Likewise.
> * c-typeck.c (handle_omp_array_sections): Adjust COMPONENT_REF case to
> use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type.
> (c_finish_omp_clauses): Adjust bitmap checks to allow struct decl and
> same struct field access to co-exist on OpenMP construct.
>
> gcc/cp/
> * parser.c (cp_parser_omp_target_data): Add use of
> new c_omp_adjust_clauses function. Add GOMP_MAP_ATTACH_DETACH as
> handled map clause kind.
> (cp_parser_omp_target_enter_data): Likewise.
> (cp_parser_omp_target_exit_data): Likewise.
> (cp_parser_omp_target): Likewise.
> * semantics.c (handle_omp_array_sections): Adjust COMPONENT_REF case to
> use GOMP_MAP_ATTACH_DETACH map kind for C_ORT_OMP region type. Fix
> interaction between reference case and attach/detach.
> (finish_omp_clauses): Adjust bitmap checks to allow struct decl and
> same struct field access to co-exist on OpenMP construct.
The changelog has some 8 space indented lines.
> + for (tree c = clauses; c; c = OMP_CLAUSE_CHAIN (c))
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
> + && OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FIRSTPRIVATE_POINTER
> + && TREE_CODE (TREE_TYPE (OMP_CLAUSE_DECL (c))) != ARRAY_TYPE)
> + {
> + tree ptr = OMP_CLAUSE_DECL (c);
> + bool ptr_mapped = false;
> + if (is_target)
> + {
> + for (tree m = clauses; m; m = OMP_CLAUSE_CHAIN (m))
Isn't this O(n^2) in number of clauses? I mean, e.g. for the equality
comparisons (but see below) it could be dealt with e.g. using some bitmap
with DECL_UIDs.
> + if (OMP_CLAUSE_CODE (m) == OMP_CLAUSE_MAP
> + && OMP_CLAUSE_DECL (m) == ptr
Does it really need to be equality? I mean it will be for
map(tofrom:ptr) map(tofrom:ptr[:32])
but what about e.g.
map(tofrom:structx) map(tofrom:structx.ptr[:32])
? It is true that likely we don't parse this yet though.
> + && (OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_ALLOC
> + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TO
> + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_FROM
> + || OMP_CLAUSE_MAP_KIND (m) == GOMP_MAP_TOFROM))
What about the always modified mapping kinds?
> + {
> + ptr_mapped = true;
> + break;
> + }
> +
> + if (!ptr_mapped
> + && DECL_P (ptr)
> + && is_global_var (ptr)
> + && lookup_attribute ("omp declare target",
> + DECL_ATTRIBUTES (ptr)))
> + ptr_mapped = true;
> + }
> +
> + /* If the pointer variable was mapped, or if this is not an offloaded
> + target region, adjust the map kind to attach/detach. */
> + if (ptr_mapped || !is_target)
> + {
> + OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_ATTACH_DETACH);
> + c_common_mark_addressable_vec (ptr);
Though perhaps this is argument why it needs to be done in the FEs and not
during gimplification, because it is hard to mark something addressable at
that point.
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -13580,16 +13580,17 @@ handle_omp_array_sections (tree c, enum
> c_omp_region_type ort)
> break;
> }
> tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c), OMP_CLAUSE_MAP);
> if (ort != C_ORT_OMP && ort != C_ORT_ACC)
> OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_POINTER);
> else if (TREE_CODE (t) == COMPONENT_REF)
> {
> - gomp_map_kind k = (ort == C_ORT_ACC) ? GOMP_MAP_ATTACH_DETACH
> - : GOMP_MAP_ALWAYS_POINTER;
> + gomp_map_kind k
> + = ((ort == C_ORT_ACC || ort == C_ORT_OMP)
> + ? GOMP_MAP_ATTACH_DETACH : GOMP_MAP_ALWAYS_POINTER);
So what kind of C_ORT_* would be left after this change?
C_ORT_*DECLARE_SIMD shouldn't have any kind of array sections in it.
So maybe just
> OMP_CLAUSE_SET_MAP_KIND (c2, k);
OMP_CLAUSE_SET_MAP_KIND (c2, GOMP_MAP_ALWAYS_POINTER);
?
> if (VAR_P (t) || TREE_CODE (t) == PARM_DECL)
> {
> - if (bitmap_bit_p (&map_field_head, DECL_UID (t)))
> + if (bitmap_bit_p (&map_field_head, DECL_UID (t))
> + || bitmap_bit_p (&map_head, DECL_UID (t)))
> break;
Shall this change apply to OpenACC too?
> }
> }
> if (!VAR_P (t) && TREE_CODE (t) != PARM_DECL)
> {
> error_at (OMP_CLAUSE_LOCATION (c),
> "%qE is not a variable in %qs clause", t,
> @@ -14751,29 +14753,36 @@ c_finish_omp_clauses (tree clauses, enum
> c_omp_region_type ort)
> error_at (OMP_CLAUSE_LOCATION (c),
> "%qD appears both in data and map clauses", t);
> remove = true;
> }
> else
> bitmap_set_bit (&generic_head, DECL_UID (t));
> }
> - else if (bitmap_bit_p (&map_head, DECL_UID (t)))
> + else if (bitmap_bit_p (&map_head, DECL_UID (t))
> + && !bitmap_bit_p (&map_field_head, DECL_UID (t)))
Ditto. Otherwise, what shall this diagnose now that the restriction that
the same list item may not appear in multiple clauses is gone.
> {
> if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP)
> error_at (OMP_CLAUSE_LOCATION (c),
> "%qD appears more than once in motion clauses", t);
> else if (ort == C_ORT_ACC)
> error_at (OMP_CLAUSE_LOCATION (c),
> "%qD appears more than once in data clauses", t);
> else
> error_at (OMP_CLAUSE_LOCATION (c),
> "%qD appears more than once in map clauses", t);
> remove = true;
So what is this supposed to diagnose now that the restriction that
C++ ditto.
Jakub
