On Tue, Sep 13, 2022 at 02:03:18PM -0700, Julian Brown wrote: > This patch is an extension and rewrite/rethink of the following two patches: > > "OpenMP/OpenACC: Add inspector class to unify mapped address analysis" > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591977.html > > "OpenMP: Handle reference-typed struct members" > https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591978.html > > The latter was reviewed here by Jakub: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595510.html with the > > with the comment, > > > Why isn't a reference to pointer handled that way too? > > and that opened a whole can of worms... generally, C++ references were > not handled very consistently after the clause-processing code had been > extended several times already for both OpenACC and OpenMP, and many > cases of using C++ (and Fortran) references were broken. Even some > cases not involving references were being mapped incorrectly. > > At present a single clause may be turned into several mapping nodes, > or have its mapping type changed, in several places scattered through > the front- and middle-end. The analysis relating to which particular > transformations are needed for some given expression has become quite hard > to follow. Briefly, we manipulate clause types in the following places: > > 1. During parsing, in c_omp_adjust_map_clauses. Depending on a set of > rules, we may change a FIRSTPRIVATE_POINTER (etc.) mapping into > ATTACH_DETACH, or mark the decl addressable. > > 2. In semantics.cc or c-typeck.cc, clauses are expanded in > handle_omp_array_sections (called via {c_}finish_omp_clauses, or in > finish_omp_clauses itself. The two cases are for processing array > sections (the former), or non-array sections (the latter). > > 3. In gimplify.cc, we build sibling lists for struct accesses, which > groups and sorts accesses along with their struct base, creating > new ALLOC/RELEASE nodes for pointers. > > 4. In gimplify.cc:gimplify_adjust_omp_clauses, mapping nodes may be > adjusted or created. > > This patch doesn't completely disrupt this scheme, though clause > types are no longer adjusted in c_omp_adjust_map_clauses (step 1). > Clause expansion in step 2 (for C and C++) now uses a single, unified > mechanism, parts of which are also reused for analysis in step 3. > > Rather than the kind-of "ad-hoc" pattern matching on addresses used to > expand clauses used at present, a new method for analysing addresses is > introduced. This does a recursive-descent tree walk on expression nodes, > and emits a vector of tokens describing each "part" of the address. > This tokenized address can then be translated directly into mapping nodes, > with the assurance that no part of the expression has been inadvertently > skipped or misinterpreted. In this way, all the variations of ways > pointers, arrays, references and component accesses can be teased apart > into easily-understood cases - and we know we've "parsed" the whole > address before we start analysis, so the right code paths can easily > be selected. > > For example, a simple access "arr[idx]" might parse as: > > base-decl access-indexed-array > > or "mystruct->foo[x]" with a pointer "foo" component might parse as: > > base-decl access-pointer component-selector access-pointer > > A key observation is that support for "array" bases, e.g. accesses > whose root nodes are not structures, but describe scalars or arrays, > and also *one-level deep* structure accesses, have first-class support > in gimplify and beyond. Expressions that use deeper struct accesses > or e.g. multiple indirections were more problematic: some cases worked, > but lots of cases didn't. This patch reimplements the support for those > in gimplify.cc, again using the new "address tokenization" support. > > An expression like "mystruct->foo->bar[0:10]" used in a mapping node will > translate the right-hand access directly in the front-end. The base for > the access will be "mystruct->foo". This is handled recursively -- there > may be several accesses of "mystruct"'s members on the same directive, > so the sibling-list building machinery can be used again. (This was > already being done for OpenACC, but the new implementation differs > somewhat in details, and is more robust.) > > For OpenMP, in the case where the base pointer itself, > i.e. "mystruct->foo" here, is NOT mapped on the same directive, we > create a "fragile" mapping. This turns the "foo" component access > into a zero-length allocation (which is a new feature for the runtime, > so support has been added there too). > > A couple of changes have been made to how mapping clauses are turned > into mapping nodes: > > The first change is based on the observation that it is probably never > correct to use GOMP_MAP_ALWAYS_POINTER for component accesses (e.g. for > references), because if the containing struct is already mapped on the > target then the host version of the pointer in question will be corrupted > if the struct is copied back from the target. This patch removes all > such uses, across each of C, C++ and Fortran. > > The second change is to the way that GOMP_MAP_ATTACH_DETACH nodes > are processed during sibling-list creation. For OpenMP, for pointer > components, we must map the base pointer separately from an array section > that uses the base pointer, so e.g. we must have both "map(mystruct.base)" > and "map(mystruct.base[0:10])" mappings. These create nodes such as: > > GOMP_MAP_TOFROM mystruct.base > G_M_TOFROM *mystruct.base [len: 10*elemsize] G_M_ATTACH_DETACH mystruct.base > > Instead of using the first of these directly when building the struct > sibling list then skipping the group using GOMP_MAP_ATTACH_DETACH, > leading to: > > GOMP_MAP_STRUCT mystruct [len: 1] GOMP_MAP_TOFROM mystruct.base > > we now introduce a new "mini-pass", omp_resolve_clause_dependencies, that > drops the GOMP_MAP_TOFROM for the base pointer, marks the second group > as having had a base-pointer mapping, then omp_build_struct_sibling_lists > can create: > > GOMP_MAP_STRUCT mystruct [len: 1] GOMP_MAP_ALLOC mystruct.base [len: > ptrsize] > > This ends up working better in many cases, particularly those involving > references. (The "alloc" space is immediately overwritten by a pointer > attachment, so this is mildly more efficient than a redundant TO mapping > at runtime also.) > > There is support in the address tokenizer for "arbitrary" base expressions > which aren't rooted at a decl, but that is not used as present because > such addresses are disallowed at parse time. > > In the front-ends, the address tokenization machinery is mostly only > used for clause expansion and not for diagnostics at present. It could > be used for those too, which would allow more of my previous "address > inspector" implementation to be removed. > > The new bits in gimplify.cc work with OpenACC also. > > 2022-09-13 Julian Brown <jul...@codesourcery.com> > > gcc/c-family/ > * c-common.h (omp_addr_token): Add forward declaration. > (c_omp_address_inspector): New class. > * c-omp.c (c_omp_adjust_map_clauses): Mark decls addressable here, but > do not change any mapping node types. > (c_omp_address_inspector::unconverted_ref_origin, > c_omp_address_inspector::component_access_p, > c_omp_address_inspector::check_clause, > c_omp_address_inspector::get_root_term, > c_omp_address_inspector::map_supported_p, > c_omp_address_inspector::get_origin, > c_omp_address_inspector::maybe_unconvert_ref, > c_omp_address_inspector::maybe_zero_length_array_section, > c_omp_address_inspector::expand_array_base, > c_omp_address_inspector::expand_component_selector, > c_omp_address_inspector::expand_map_clause): New methods. > (omp_expand_access_chain): New function. > > gcc/c/ > * c-typeck.c (handle_omp_array_sections_1, > handle_omp_array_sections, c_finish_omp_clauses): Use > c_omp_address_inspector class and OMP address tokenizer to analyze and > expand map clause expressions. Fix some diagnostics. > > gcc/cp/ > * semantics.c (cp_omp_address_inspector): New class, derived from > c_omp_address_inspector. > (handle_omp_array_sections_1, handle_omp_array_sections, > finish_omp_clauses): Use cp_omp_address_inspector class and OMP address > tokenizer to analyze and expand OpenMP map clause expressions. Fix > some diagnostics. > > gcc/fortran/ > * trans-openmp.cc (gfc_trans_omp_array_section): Add OPENMP parameter. > Use GOMP_MAP_ATTACH_DETACH instead of GOMP_MAP_ALWAYS_POINTER for > derived type components. > (gfc_trans_omp_clauses): Update calls to gfc_trans_omp_array_section. > > gcc/ > * gimplify.cc (build_struct_comp_nodes): Don't process > GOMP_MAP_ATTACH_DETACH "middle" nodes here. > (omp_mapping_group): Add REPROCESS_STRUCT and FRAGILE booleans for > nested struct handling. > (omp_strip_components_and_deref, omp_strip_indirections): Remove > functions. > (omp_gather_mapping_groups_1): Initialise reprocess_struct and fragile > fields. > (omp_group_base): Handle GOMP_MAP_ATTACH_DETACH after GOMP_MAP_STRUCT. > (omp_index_mapping_groups_1): Skip reprocess_struct groups. > (omp_get_nonfirstprivate_group, omp_directive_maps_explicitly, > omp_resolve_clause_dependencies, omp_expand_access_chain): New > functions. > (omp_accumulate_sibling_list): Add GROUP_MAP, ADDR_TOKENS, FRAGILE_P, > REPROCESSING_STRUCT, ADDED_TAIL parameters. Use OMP address tokenizer > to analyze addresses. Reimplement nested struct handling, and > implement "fragile groups". > (omp_build_struct_sibling_lists): Adjust for changes to > omp_accumulate_sibling_list. Recalculate bias for ATTACH_DETACH nodes > after GOMP_MAP_STRUCT nodes. > (gimplify_scan_omp_clauses): Call omp_resolve_clause_dependencies. Use > OMP address tokenizer. > (gimplify_adjust_omp_clauses_1): Use build_fold_indirect_ref_loc > instead of build_simple_mem_ref_loc. > * omp-general.cc (omp-general.h, tree-pretty-print.h): Include. > (omp_addr_tokenizer): New namespace. > (omp_addr_tokenizer::omp_addr_token): New. > (omp_addr_tokenizer::omp_parse_component_selector, > omp_addr_tokenizer::omp_parse_ref, > omp_addr_tokenizer::omp_parse_pointer, > omp_addr_tokenizer::omp_parse_access_method, > omp_addr_tokenizer::omp_parse_access_methods, > omp_addr_tokenizer::omp_parse_structure_base, > omp_addr_tokenizer::omp_parse_structured_expr, > omp_addr_tokenizer::omp_parse_array_expr, > omp_addr_tokenizer::omp_access_chain_p, > omp_addr_tokenizer::omp_accessed_addr: New functions. > (omp_parse_expr, debug_omp_tokenized_addr): New functions. > * omp-general.h (omp_addr_tokenizer::access_method_kinds, > omp_addr_tokenizer::structure_base_kinds, > omp_addr_tokenizer::token_type, > omp_addr_tokenizer::omp_addr_token, > omp_addr_tokenizer::omp_access_chain_p, > omp_addr_tokenizer::omp_accessed_addr): New. > (omp_addr_token, omp_parse_expr): New. > * omp-low.cc (scan_sharing_clauses): Skip error check for references > to pointers. > * tree.h (OMP_CLAUSE_ATTACHMENT_MAPPING_ERASED): New macro. > > gcc/testsuite/ > * c-c++-common/goacc/mdc-2.c: Update expected errors. > * c-c++-common/gomp/clauses-2.c: Fix error output. > * c-c++-common/gomp/target-implicit-map-2.c: Adjust scan output. > * g++.dg/gomp/unmappable-component-1.C: New test. > * g++.dg/goacc/mdc.C: Update expected errors. > * g++.dg/gomp/static-component-1.C: New test. > * gcc.dg/gomp/target-3.c: Adjust scan output. > > libgomp/ > * target.c (gomp_map_fields_existing: Use gomp_map_0len_lookup. > (gomp_attach_pointer): Allow attaching null pointers (or Fortran > "unassociated" pointers). > (gomp_map_vars_internal): Handle zero-sized struct members. Add > diagnostic for unmapped struct pointer members. > * testsuite/libgomp.c++/class-array-1.C: New test. > * testsuite/libgomp.c-c++-common/baseptrs-1.c: New test. > * testsuite/libgomp.c-c++-common/baseptrs-2.c: New test. > * testsuite/libgomp.c++/baseptrs-3.C: New test. > * testsuite/libgomp.c++/baseptrs-4.C: New test. > * testsuite/libgomp.c++/baseptrs-5.C: New test. > * testsuite/libgomp.c++/target-48.C: New test. > * testsuite/libgomp.c++/target-49.C: New test. > * testsuite/libgomp.c/target-22.c: Add necessary explicit base pointer > mappings. > --- > gcc/c-family/c-common.h | 68 + > gcc/c-family/c-omp.cc | 765 +++- > gcc/c/c-typeck.cc | 363 +- > gcc/cp/semantics.cc | 568 ++- > gcc/fortran/trans-openmp.cc | 36 +- > gcc/gimplify.cc | 1030 +++++- > gcc/omp-general.cc | 426 +++ > gcc/omp-general.h | 57 + > gcc/omp-low.cc | 7 +- > gcc/testsuite/c-c++-common/goacc/mdc-2.c | 2 + > gcc/testsuite/c-c++-common/gomp/clauses-2.c | 2 +- > .../c-c++-common/gomp/target-implicit-map-2.c | 2 +- > gcc/testsuite/g++.dg/goacc/mdc.C | 2 + > .../g++.dg/gomp/static-component-1.C | 23 + > gcc/testsuite/gcc.dg/gomp/target-3.c | 2 +- > gcc/tree.h | 4 + > libgomp/target.c | 31 +- > libgomp/testsuite/libgomp.c++/baseptrs-3.C | 275 ++ > libgomp/testsuite/libgomp.c++/baseptrs-4.C | 3154 +++++++++++++++++ > libgomp/testsuite/libgomp.c++/baseptrs-5.C | 62 + > libgomp/testsuite/libgomp.c++/class-array-1.C | 59 + > libgomp/testsuite/libgomp.c++/target-48.C | 32 + > libgomp/testsuite/libgomp.c++/target-49.C | 37 + > .../libgomp.c-c++-common/baseptrs-1.c | 50 + > .../libgomp.c-c++-common/baseptrs-2.c | 70 + > libgomp/testsuite/libgomp.c/target-22.c | 3 +- > 26 files changed, 6394 insertions(+), 736 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/gomp/static-component-1.C > create mode 100644 libgomp/testsuite/libgomp.c++/baseptrs-3.C > create mode 100644 libgomp/testsuite/libgomp.c++/baseptrs-4.C > create mode 100644 libgomp/testsuite/libgomp.c++/baseptrs-5.C > create mode 100644 libgomp/testsuite/libgomp.c++/class-array-1.C > create mode 100644 libgomp/testsuite/libgomp.c++/target-48.C > create mode 100644 libgomp/testsuite/libgomp.c++/target-49.C > create mode 100644 libgomp/testsuite/libgomp.c-c++-common/baseptrs-1.c > create mode 100644 libgomp/testsuite/libgomp.c-c++-common/baseptrs-2.c > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index e7b0fd1309d..4ea244b3ce9 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1270,6 +1270,74 @@ extern tree c_omp_check_context_selector (location_t, > tree); > extern void c_omp_mark_declare_variant (location_t, tree, tree); > extern void c_omp_adjust_map_clauses (tree, bool); > > +namespace omp_addr_tokenizer { struct omp_addr_token; } > +typedef omp_addr_tokenizer::omp_addr_token omp_addr_token; > + > +class c_omp_address_inspector > +{ > + location_t loc; > + tree root_term; > + bool indirections; > + int map_supported; > + > +protected: > + tree orig; > + > +public: > + c_omp_address_inspector (location_t loc, tree t) > + : loc (loc), root_term (NULL_TREE), indirections (false), > + map_supported (-1), orig (t) > + {} > + > + ~c_omp_address_inspector () > + {} > + > + virtual bool processing_template_decl_p () > + { > + return false; > + } > + > + virtual void emit_unmappable_type_notes (tree) > + { > + } > + > + virtual tree convert_from_reference (tree) > + { > + gcc_unreachable (); > + } > + > + virtual tree build_array_ref (location_t loc, tree arr, tree idx) > + { > + tree eltype = TREE_TYPE (TREE_TYPE (arr)); > + return build4_loc (loc, ARRAY_REF, eltype, arr, idx, NULL_TREE, > + NULL_TREE); > + } > + > + bool check_clause (tree); > + tree get_root_term (bool); > + > + tree get_address () > + { > + return orig; > + }
This has the method formatting style inconsistency I've walked about earlier. Either the {s are indented 2 further columns, or they aren't, but definitely not both in the same class. Missing function comment before following: > +static bool > +omp_directive_maps_explicitly (hash_map<tree_operand_hash, > + omp_mapping_group *> *grpmap, > + tree decl, omp_mapping_group **base_group, > + bool to_specifically, bool allow_deleted, > + bool contained_in_struct) > +{ > + omp_mapping_group *decl_group > + = omp_get_nonfirstprivate_group (grpmap, decl, allow_deleted); > + > + *base_group = NULL; > + > + if (decl_group) > + { > + tree grp_first = *decl_group->grp_start; > + /* We might be called during omp_build_struct_sibling_lists, when > + GOMP_MAP_STRUCT might have been inserted at the start of the group. > + Skip over that, and also possibly the node after it. */ > + if (OMP_CLAUSE_MAP_KIND (grp_first) == GOMP_MAP_STRUCT) > + { > + grp_first = OMP_CLAUSE_CHAIN (grp_first); > + if (OMP_CLAUSE_MAP_KIND (grp_first) == GOMP_MAP_FIRSTPRIVATE_POINTER > + || (OMP_CLAUSE_MAP_KIND (grp_first) > + == GOMP_MAP_FIRSTPRIVATE_REFERENCE) > + || OMP_CLAUSE_MAP_KIND (grp_first) == GOMP_MAP_ATTACH_DETACH) > + grp_first = OMP_CLAUSE_CHAIN (grp_first); > + } > + enum gomp_map_kind first_kind = OMP_CLAUSE_MAP_KIND (grp_first); > + if (!to_specifically > + || GOMP_MAP_COPY_TO_P (first_kind) > + || first_kind == GOMP_MAP_ALLOC) > + { > + *base_group = decl_group; > + return true; > + } > + } > + > + if (contained_in_struct > + && omp_mapped_by_containing_struct (grpmap, decl, base_group)) > + return true; > + > + return false; > +} Why? gimplify_scan_omp_clauses certainly should have a function comment. > > -/* Scan the OMP clauses in *LIST_P, installing mappings into a new > - and previous omp contexts. */ > - > static void > gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p, > enum omp_region_type region_type, > enum tree_code code) > + > +enum structure_base_kinds > +{ > + BASE_DECL, > + BASE_COMPONENT_EXPR, > + BASE_ARBITRARY_EXPR > +}; > + > +enum token_type > +{ > + ARRAY_BASE, > + STRUCTURE_BASE, > + COMPONENT_SELECTOR, > + ACCESS_METHOD > +}; Wouldn't hurt to add comment about omp_addr_token and the above two enums. > + > +struct omp_addr_token > +{ > + enum token_type type; > + tree expr; > + > + union > + { > + access_method_kinds access_kind; > + structure_base_kinds structure_base_kind; > + } u; > + > + omp_addr_token (token_type, tree); > + omp_addr_token (access_method_kinds, tree); > + omp_addr_token (token_type, structure_base_kinds, tree); > +}; > --- a/libgomp/target.c > +++ b/libgomp/target.c > @@ -718,7 +718,7 @@ gomp_map_fields_existing (struct target_mem_desc *tgt, > > cur_node.host_start = (uintptr_t) hostaddrs[i]; > cur_node.host_end = cur_node.host_start + sizes[i]; > - splay_tree_key n2 = splay_tree_lookup (mem_map, &cur_node); > + splay_tree_key n2 = gomp_map_0len_lookup (mem_map, &cur_node); > kind = get_kind (short_mapkind, kinds, i); > implicit = get_implicit (short_mapkind, kinds, i); > if (n2 I'm a little bit worried if the above isn't a backwards incompatible change. We need programs compiled by GCC 12 and earlier to be handled correctly. > --- /dev/null > +++ b/libgomp/testsuite/libgomp.c++/baseptrs-4.C > @@ -0,0 +1,3154 @@ > +// { dg-do run } > + > +#include <cstring> > +#include <cassert> > + > +#define MAP_DECLS > + > +#define NONREF_DECL_BASE > +#define REF_DECL_BASE > +#define PTR_DECL_BASE > +#define REF2PTR_DECL_BASE > + > +#define ARRAY_DECL_BASE > +// Needs map clause "lvalue"-parsing support. > +//#define REF2ARRAY_DECL_BASE > +#define PTR_OFFSET_DECL_BASE > +// Needs map clause "lvalue"-parsing support. > +//#define REF2PTR_OFFSET_DECL_BASE > + > +#define MAP_SECTIONS > + > +#define NONREF_DECL_MEMBER_SLICE > +#define NONREF_DECL_MEMBER_SLICE_BASEPTR > +#define REF_DECL_MEMBER_SLICE > +#define REF_DECL_MEMBER_SLICE_BASEPTR > +#define PTR_DECL_MEMBER_SLICE > +#define PTR_DECL_MEMBER_SLICE_BASEPTR > +#define REF2PTR_DECL_MEMBER_SLICE > +#define REF2PTR_DECL_MEMBER_SLICE_BASEPTR > + > +#define ARRAY_DECL_MEMBER_SLICE > +#define ARRAY_DECL_MEMBER_SLICE_BASEPTR > +// Needs map clause "lvalue"-parsing support. > +//#define REF2ARRAY_DECL_MEMBER_SLICE > +//#define REF2ARRAY_DECL_MEMBER_SLICE_BASEPTR > +#define PTR_OFFSET_DECL_MEMBER_SLICE > +#define PTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR > +// Needs map clause "lvalue"-parsing support. > +//#define REF2PTR_OFFSET_DECL_MEMBER_SLICE > +//#define REF2PTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR > + > +#define PTRARRAY_DECL_MEMBER_SLICE > +#define PTRARRAY_DECL_MEMBER_SLICE_BASEPTR > +// Needs map clause "lvalue"-parsing support. > +//#define REF2PTRARRAY_DECL_MEMBER_SLICE > +//#define REF2PTRARRAY_DECL_MEMBER_SLICE_BASEPTR > +#define PTRPTR_OFFSET_DECL_MEMBER_SLICE > +#define PTRPTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR > +// Needs map clause "lvalue"-parsing support. > +//#define REF2PTRPTR_OFFSET_DECL_MEMBER_SLICE > +//#define REF2PTRPTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR > + > +#define NONREF_COMPONENT_BASE > +#define NONREF_COMPONENT_MEMBER_SLICE > +#define NONREF_COMPONENT_MEMBER_SLICE_BASEPTR > + > +#define REF_COMPONENT_BASE > +#define REF_COMPONENT_MEMBER_SLICE > +#define REF_COMPONENT_MEMBER_SLICE_BASEPTR > + > +#define PTR_COMPONENT_BASE > +#define PTR_COMPONENT_MEMBER_SLICE > +#define PTR_COMPONENT_MEMBER_SLICE_BASEPTR > + > +#define REF2PTR_COMPONENT_BASE > +#define REF2PTR_COMPONENT_MEMBER_SLICE > +#define REF2PTR_COMPONENT_MEMBER_SLICE_BASEPTR Are all these defines only a temporary hacks until the patchset is complete? Jakub