Hi Jakub, Thanks for review! On Tue, 24 May 2022 15:03:07 +0200 Jakub Jelinek via Fortran wrote: > On Fri, Mar 18, 2022 at 09:24:51AM -0700, Julian Brown wrote: > > 2021-11-23 Julian Brown > > > > gcc/ > > * gimplify.c (is_or_contains_p, > > omp_target_reorder_clauses): Delete functions. > > (omp_tsort_mark): Add enum. > > (omp_mapping_group): Add struct. > > (debug_mapping_group, omp_get_base_pointer, > > omp_get_attachment, omp_group_last, omp_gather_mapping_groups, > > omp_group_base, omp_index_mapping_groups, omp_containing_struct, > > omp_tsort_mapping_groups_1, omp_tsort_mapping_groups, > > omp_segregate_mapping_groups, omp_reorder_mapping_groups): > > New functions. > > (gimplify_scan_omp_clauses): Call above functions instead of > > omp_target_reorder_clauses, unless we've seen an error. > > * omp-low.c (scan_sharing_clauses): Avoid strict test if we > > haven't sorted mapping groups. > > > > gcc/testsuite/ > > * g++.dg/gomp/target-lambda-1.C: Adjust expected output. > > * g++.dg/gomp/target-this-3.C: Likewise. > > * g++.dg/gomp/target-this-4.C: Likewise. > > + > > Wouldn't hurt to add a comment on the meanings of the enumerators. Added. > > +enum omp_tsort_mark { > > + UNVISITED, > > + TEMPORARY, > > + PERMANENT > > +}; > > + > > +struct omp_mapping_group { > > + tree *grp_start; > > + tree grp_end; > > + omp_tsort_mark mark; > > + struct omp_mapping_group *sibling; > > + struct omp_mapping_group *next; > > +}; > > + > > +__attribute__((used)) static void > > I'd use what is used elsewhere, > DEBUG_FUNCTION void > without static. Fixed. > > +static tree > > +omp_get_base_pointer (tree expr) > I must say I don't see advantages of just a single loop that > looks through all ARRAY_REFs and all COMPONENT_REFs and then just > checks if the expr it got in the end is a decl or INDIRECT_REF > or MEM_REF with offset 0. > > > +static tree > > +omp_containing_struct (tree expr) > Again? I've simplified these loops, and removed the "needs improvement" comment. > > @@ -9063,11 +9820,29 @@ 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) > > - omp_target_reorder_clauses (list_p); > > + /* Topological sorting may fail if we have duplicate nodes, which > > + we should have detected and shown an error for already. Skip > > + sorting in that case. */ > > + if (!seen_error () > > + && (code == OMP_TARGET > > + || code == OMP_TARGET_DATA > > + || code == OMP_TARGET_ENTER_DATA > > + || code == OMP_TARGET_EXIT_DATA)) > > + { > > + vec *groups; > > + groups = omp_gather_mapping_groups (list_p); > > + if (groups) > > + { > > + hash_map *grpmap; > > + grpmap = omp_index_mapping_groups (groups); > > + omp_mapping_group *outlist > > + = omp_tsort_mapping_groups (groups, grpmap); > > + outlist = omp_segregate_mapping_groups (outlist); > > + list_p = omp_reorder_mapping_groups (groups, outlist, > > list_p); > > + delete grpmap; > > + delete groups; > > + } > > + } > > I think big question is if we do want to do this map clause reordering > before processing the omp target etc. clauses, or after (during > gimplify_adjust_omp_clauses, when clauses from the implicit mappings > are added too and especially with the declare mapper expansions), > or both before and after. The existing code constrains us a bit here, unless we want to completely rewrite it! We can only do sorting on clauses before gimplification, otherwise the "structural" matching of the parsed syntax of base pointers inside other clauses on the directive, etc. will certainly fail. (Semi-relatedly, I asked this on the omp-lang mailing list: "When we have mappings that represent base pointers, and other mappings that use those base pointers, the former must be ordered to take place before the latter -- but should we determine that relation purely syntactically? How about if we write e.g. "p->" on one vs. "(*p)." on the other?" but no reply...) So, this is fine for sorting explicit mapping clauses. When planning the approach I've used for "declare mapper" support, I wrote this (in an internal email): "At the moment, gimplifying OMP workshare regions proceeds in three phases: 1. Clauses are processed (gimplify_scan_omp_clauses), creating records of mapped variables in a splay tree, with associated flags. 2. The body of the workshare region is processed (gimplified), augmenting the same splay tree with information about variables which are used implicitly (and maybe also modifying the "explicit" mappings from the first step). 3. The clauses are modified based on the results of the second stage (gimplify_adjust_omp_clauses). E.g. clauses are removed that refer to variables that aren't actually used in the region, or new clauses created for implicitly-referenced variables without mapping clauses on the construct. The problem with this with regards to mappers is that the "expanded" mappers should undergo some of the processing we currently perform during phase 1 (struct sibling list handling, and so on), but we don't know which variables are implicitly referenced until phase 2. [description of a plan that didn't work removed] So the new plan is to do: phase 1 (scan original clauses) phase 2 (scan workshare body) phase 1 (use variables from "2" to instantiate mappers, and process new clauses only. Prepend new list to original clauses) phase 3 (as before) I was concerned that this would upset the sorting code -- but I think actually, as long as implicitly-created clauses are inserted at the front of the clause list, there can't be a case where a pointer base is mapped after a use of that base. If that assumption turns out to be wrong, then things might get a little more complicated." ...and so far, the plan seems to be working out. The assumption, to state it in other words, is that an implicitly-added map clause *cannot* have a dependency on an explicit map clause, in terms of relying on a base pointer in that explicit clause, by construction. > > while ((c = *list_p) != NULL) > > { > > diff --git a/gcc/omp-low.cc b/gcc/omp-low.cc > > index c33b3daa439..ffeb1f34fd7 100644 > > --- a/gcc/omp-low.cc > > +++ b/gcc/omp-low.cc > > @@ -1537,8 +1537,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 ()) > > If we encounter a major error during processing map clauses, we > should consider just leaving out the offloading construct from the IL. I experimented with that idea, but I think it may be a much more invasive change (I saw lots of testsuite fall-out relating to things that no longer raise "cascaded" errors, though maybe the approach I took was too crude). I think if we want to do that, it's probably better handled with a separate patch. I've re-tested the attached with offloading to NVPTX. OK? Thanks, Julian