From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 9FDFD385737A; Wed, 14 Sep 2022 13:59:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9FDFD385737A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.93,315,1654588800"; d="scan'208";a="85747682" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 14 Sep 2022 05:59:17 -0800 IronPort-SDR: 7TteIPdcF/mQCTOKVl3TEaU2xBREdJqHtdugyVKiFqkkE589BYsGl6QsjelP03gmVy8u3/obIQ DKw82ESOOAkI3OxT7OkwU0TN6nFi+39JOYlNGyVmZvwi9/myg8Ob7v3VusBICMm2KplxSaZYoG 6Cc2f7vBfDw2v66lbMeoUyRfP1zva+0yo7vbvUWC7PoJUsLUXwDjY0MANaym2i8qaucdcfHJFm 18QdavqFGKolLLw4lGR9q1LNVtrYE5fDHfgQBxhkbyoJvQmANC32aQAwxPBgwfKGN2hWQo0Nrn BNw= Date: Wed, 14 Sep 2022 14:59:03 +0100 From: Julian Brown To: Jakub Jelinek via Fortran CC: Jakub Jelinek , , , Subject: Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling Message-ID: <20220914145903.4fee1fb5@squid.athome> In-Reply-To: References: Organization: Mentor Graphics X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: SVR-ORW-MBX-07.mgc.mentorg.com (147.34.90.207) To svr-ies-mbx-11.mgc.mentorg.com (139.181.222.11) X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, 14 Sep 2022 15:24:12 +0200 Jakub Jelinek via Fortran wrote: > On Tue, Sep 13, 2022 at 02:03:18PM -0700, Julian Brown wrote: > > +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. OK, I wasn't sure about that (despite your previous comment), since the empty constructor/destructor un-indented {} is present elsewhere in GCC (...though I think you mentioned that, too). Will fix. > Missing function comment before following: > > > +static bool > > +omp_directive_maps_explicitly (hash_map > + 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. I'm actually not sure what happened there -- a merge error, I think. Sorry about that! > > > > -/* 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. I think that should be fine for that change -- zero-length fields won't be present for GCC 12-compiled code, and if they were, it'd be a runtime crash. > > --- /dev/null > > +++ b/libgomp/testsuite/libgomp.c++/baseptrs-4.C > > @@ -0,0 +1,3154 @@ > > +// { dg-do run } > > + > > +#include > > +#include > > + > > +#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? Yes -- those commented-out bits are re-enabled by the following patch ("FYI/unfinished: OpenMP: lvalue parsing for map clauses (C++)"), though that one still has previous review comments to be addressed also. Thanks, Julian