Re: [PATCH v3 01/11] OpenMP 5.0: Clause ordering for OpenMP 5.0 (topological sorting by base pointer)
On Tue, Sep 13, 2022 at 02:01:42PM -0700, Julian Brown wrote:
> --- a/gcc/omp-low.cc
> +++ b/gcc/omp-low.cc
> @@ -1599,8 +1599,11 @@ scan_sharing_clauses (tree clauses, omp_context *ctx)
> {
> /* If this is an offloaded region, an attach operation should
>only exist when the pointer variable is mapped in a prior
> - clause. */
> - if (is_gimple_omp_offloaded (ctx->stmt))
> + clause.
> + If we had an error, we may not have attempted to sort clauses
> + properly, so avoid the test. */
> + if (is_gimple_omp_offloaded (ctx->stmt)
> + && !seen_error ())
I'll repeat that it would be better if we just leave out the whole
GOMP_TARGET if there were errors regarding it and the clauses aren't sorted
properly.
But I'm ok if it is handled incrementally and this spot reverted.
Otherwise LGTM.
Jakub
Re: [PATCH v3 02/11] Remove omp_target_reorder_clauses
On Tue, Sep 13, 2022 at 02:01:43PM -0700, Julian Brown wrote: > This patch has been split out from the previous one to avoid a > confusingly-interleaved diff. The two patches will be committed squashed > together. > > 2022-09-13 Julian Brown > > gcc/ > * gimplify.c (omp_target_reorder_clauses): Delete. Ok. Jakub
Re: [PATCH v3 03/11] OpenMP/OpenACC struct sibling list gimplification extension and rework
On Tue, Sep 13, 2022 at 02:01:44PM -0700, Julian Brown wrote: > static tree > -insert_struct_comp_map (enum tree_code code, tree c, tree struct_node, > - tree prev_node, tree *scp) > +build_struct_comp_nodes (enum tree_code code, tree grp_start, tree grp_end, > + tree *extra_node) I'd rename even this function, say to build_omp_struct_comp_nodes. So that it is clear it is OpenMP/OpenACC specific. Otherwise LGTM. Jakub
Re: [PATCH v3 04/11] OpenMP/OpenACC: mapping group list-handling improvements
On Tue, Sep 13, 2022 at 02:01:45PM -0700, Julian Brown wrote: > @@ -9443,6 +9499,41 @@ omp_containing_struct (tree expr) >return expr; > } > Missing function comment here. > +static bool > +omp_mapped_by_containing_struct (hash_map + omp_mapping_group *> *grpmap, > + tree decl, > + omp_mapping_group **mapped_by_group) Otherwise LGTM. Jakub
Re: [PATCH v3 05/11] OpenMP: push attaches to end of clause list in "target" regions
On Tue, Sep 13, 2022 at 02:03:15PM -0700, Julian Brown wrote:
> This patch moves GOMP_MAP_ATTACH{_ZERO_LENGTH_ARRAY_SECTION} nodes to
> the end of the clause list, for offload regions. This ensures that when
> we do the attach operation, both the "attachment point" and the target
> region have both already been mapped on the target. This avoids a
> pathological case that can otherwise happen with struct sibling-list
> handling.
>
> 2022-09-13 Julian Brown
>
> gcc/
> * gimplify.cc (omp_segregate_mapping_groups): Update comment.
> (omp_push_attaches_to_end): New function.
> (gimplify_scan_omp_clauses): Use omp_push_attaches_to_end for offloaded
> regions.
Shouldn't this be done at the end of gimplify_adjust_omp_clauses?
I mean, can't further attach clauses appear because of declare mapper
for implicitly mapped variables?
Other than that, it is yet another walk of the whole clause list, so would
be nicer if it could be done in an existing walk over the clauses or
at least have a flag whether there are any such clauses present and do it
only in that case.
If it could be done in the main gimplify_adjust_omp_clauses loop, nice,
if it can appear also during
splay_tree_foreach (ctx->variables, gimplify_adjust_omp_clauses_1, &data);
that isn't the case.
Jakub
Re: [PATCH v3 06/11] OpenMP: Pointers and member mappings
On Tue, Sep 13, 2022 at 02:03:16PM -0700, Julian Brown wrote:
> @@ -3440,6 +3437,50 @@ gfc_trans_omp_clauses (stmtblock_t *block,
> gfc_omp_clauses *clauses,
> {
> if (pointer || (openacc && allocatable))
> {
> + gfc_omp_namelist *n2
> + = openacc ? NULL : clauses->lists[OMP_LIST_MAP];
> +
> + /* If the last reference is a pointer to a derived
> + type ("foo%dt_ptr"), check if any subcomponents
> + of the same derived type member are being mapped
> + elsewhere in the clause list ("foo%dt_ptr%x",
> + etc.). If we have such subcomponent mappings,
> + we only create an ALLOC node for the pointer
> + itself, and inhibit mapping the whole derived
> + type. */
> +
> + for (; n2 != NULL; n2 = n2->next)
> + {
> + if (n == n2 || !n2->expr)
> + continue;
> +
> + int dep
> + = gfc_dep_resolver (n->expr->ref, n2->expr->ref,
> + NULL, true);
> + if (dep == 0)
> + continue;
Isn't this and the other loop quadratic compile time in number of clauses?
Could it be done linearly through some perhaps lazily built hash table or
something similar?
Jakub
Re: [PATCH v3 07/11] OpenMP/OpenACC: Reindent TO/FROM/_CACHE_ stanza in {c_}finish_omp_clause
On Tue, Sep 13, 2022 at 02:03:17PM -0700, Julian Brown wrote: > This patch trivially adds braces and reindents the > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza in > c_finish_omp_clause and finish_omp_clause, in preparation for the > following patch (to clarify the diff a little). > > 2022-09-13 Julian Brown > > gcc/c/ > * c-typeck.cc (c_finish_omp_clauses): Add braces and reindent > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza. > > gcc/cp/ > * semantics.cc (finish_omp_clause): Add braces and reindent > OMP_CLAUSE_TO/OMP_CLAUSE_FROM/OMP_CLAUSE__CACHE_ stanza. Not very happy about this because it ruins git blame, some vars can be declared separately from initializing them and be thus usable in switches. But I see you use there classes with ctors... So ok. Jakub
Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling
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
Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling
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_no
Re: [PATCH v3 11/11] FYI/unfinished: OpenMP 5.0 "declare mapper" support for C++
On Tue, Sep 13, 2022 at 02:04:30PM -0700, Julian Brown wrote:
> This patch implements OpenMP 5.0 "declare mapper" support for C++.
> This hasn't been fully revised yet following previous review comments,
> but I am including it in this series to demonstrate the new approach to
> gimplifying map clauses after "declare mapper" instantiation.
>
> The "gimplify_scan_omp_clauses" function is still called twice: firstly
> (before scanning the offload region's body) with SUPPRESS_GIMPLIFICATION
> set to true. This sets up variables in the splay tree, etc. but does
> not gimplify anything.
>
> Then, implicit "declare mappers" are instantiated after scanning the
> region's body, then "gimplify_scan_omp_clauses" is called again, and
> does the rest of its previous tasks -- builds struct sibling lists,
> and gimplifies clauses. Then gimplify_adjust_omp_clauses is called,
> and compilation continues.
>
> Does this approach seem OK?
As I wrote in https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596444.html
I don't see a reason for this 3 passes approach and it will be a
maintainance problem.
The reason why we have 2 passes approach is that we need to populate the
splay trees based on explicit clauses before we gimplify the body, at which
point we can look up for variables seen in the body those splay trees,
either mark the explicit ones as seen or create new ones for implicit
etc. And finally we need to adjust some explicit clauses based on that
(that is what the main loop in gimplify_adjust_omp_clauses does) and
add new clauses for implicit data sharing or mapping (that is done
through splay tree traversal through gimplify_adjust_omp_clauses_1).
We also need to gimplify expressions from the clauses somewhere, but
due to the way the gimplifier works we don't care that much when
exactly it is done, it can be done before the body is gimplified
(for most clauses we do it there in gimplify_scan_omp_clauses), it can be
done after the body is gimplified, the expressions from the clauses
will be in either case gimplified into some gimple sequence that we'll
place before the construct with its body. The only reason to have
the gimplification done before and not after would be if the temporaries
from the gimplification are then needed in the splay trees for the
gimplification of the body.
I'd strongly prefer if the gimplification APIs for all the constructs
were just gimplify_scan_omp_clauses before processing the body and
gimplify_adjust_omp_clauses after doing so, not some extra APIs.
If there is a strong reason for 3 or more passes on say a subset of clauses,
either gimplify_scan_omp_clauses or gimplify_adjust_omp_clauses can do more
than one loop over the clauses, but those secondary loops ideally should be
enabled only when needed (e.g. see the gimplify_adjust_omp_clauses
has_inscan_reductions guarded loop at the end) and should only process
clauses they strictly have to.
Conceptually, there is no reason why e.g. the gimplification of the explicit
map clauses can't be done in gimplify_adjust_omp_clauses rather than in
gimplify_scan_omp_clauses. What needs to happen in gimplify_scan_omp_clauses
is just what affects the gimplification of the body. Does sorting of the
map clause affect it? I don't think so. Does declare mapper processing of
the explicit map clauses affect it? I very much hope it doesn't, but I'm
afraid I don't remember all the declare mapper restrictions and discussions.
Can declare mapper e.g. try to map an unrelated variable in addition to say
parts of the structure? If yes, then it could affect the gimplification of
the body, say
struct S { int s, t; };
extern int y;
#pragma omp declare mapper (struct S s) map (to: s.s) map (to: y)
#pragma omp target defaultmap(none:scalar) map(tofrom: x)
{
int x = s.s + y;
}
because if we do process declare mapper before the gimplification of the
body, then y would be explicitly mapped, but if we don't, it wouldn't and
it should be rejected. But in that case we'd be in big trouble with
implicit mappings using that same declare mapper. Because it would then be
significant in which order we process the vars during gimplification of the
body and whether we process declare mapper right away at that point or only
at the end etc.
We have the
"List items in map clauses on the declare mapper directive may only refer to
the declared
variable var and entities that could be referenced by a procedure defined at
the same
location."
restriction but not sure that says the above isn't valid.
So probably it needs to be discussed in omp-lang.
If the agreement is that declare mapper for explicit map clauses needs to be
done before processing the body and declare mapper for implicit map clauses
can be deferred to the end, then yes, we need to handle declare mapper
twice, but it can be done say in a secondary loop of gimplify_scan_omp_clauses
guarded on at least one of the map clauses needs declare mapper processing
or something similar that can be quickly determined
Re: [PATCH v3 11/11] FYI/unfinished: OpenMP 5.0 "declare mapper" support for C++
On Wed, 14 Sep 2022 16:58:28 +0200 Jakub Jelinek wrote: > On Tue, Sep 13, 2022 at 02:04:30PM -0700, Julian Brown wrote: > > This patch implements OpenMP 5.0 "declare mapper" support for C++. > > This hasn't been fully revised yet following previous review > > comments, but I am including it in this series to demonstrate the > > new approach to gimplifying map clauses after "declare mapper" > > instantiation. > > > > The "gimplify_scan_omp_clauses" function is still called twice: > > firstly (before scanning the offload region's body) with > > SUPPRESS_GIMPLIFICATION set to true. This sets up variables in the > > splay tree, etc. but does not gimplify anything. > > > > Then, implicit "declare mappers" are instantiated after scanning the > > region's body, then "gimplify_scan_omp_clauses" is called again, and > > does the rest of its previous tasks -- builds struct sibling lists, > > and gimplifies clauses. Then gimplify_adjust_omp_clauses is called, > > and compilation continues. > > > > Does this approach seem OK? > > As I wrote in > https://gcc.gnu.org/pipermail/gcc-patches/2022-June/596444.html I > don't see a reason for this 3 passes approach and it will be a > maintainance problem. Ack. I'll have another go at refactoring those bits when reworking this patch. Thanks, Julian
