On Fri, Mar 18, 2022 at 09:24:51AM -0700, Julian Brown wrote: > 2021-11-23 Julian Brown <jul...@codesourcery.com> > > 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. > +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. > +debug_mapping_group (omp_mapping_group *grp) > +{ > + tree tmp = OMP_CLAUSE_CHAIN (grp->grp_end); > + OMP_CLAUSE_CHAIN (grp->grp_end) = NULL; > + debug_generic_expr (*grp->grp_start); > + OMP_CLAUSE_CHAIN (grp->grp_end) = tmp; > +} > + > +/* Return the OpenMP "base pointer" of an expression EXPR, or NULL if there > + isn't one. This needs improvement. */ > + > +static tree > +omp_get_base_pointer (tree expr) > +{ > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + while (TREE_CODE (expr) == COMPONENT_REF > + && (DECL_P (TREE_OPERAND (expr, 0)) > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == COMPONENT_REF) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == INDIRECT_REF > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == MEM_REF > + && integer_zerop (TREE_OPERAND (TREE_OPERAND (expr, 0), 1))) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == ARRAY_REF)) > + { > + expr = TREE_OPERAND (expr, 0); > + > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + if (TREE_CODE (expr) == INDIRECT_REF || TREE_CODE (expr) == MEM_REF) > + break; > + } 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. > + if (DECL_P (expr)) > + return NULL_TREE; > + > + if (TREE_CODE (expr) == INDIRECT_REF > + || TREE_CODE (expr) == MEM_REF) > + { > + expr = TREE_OPERAND (expr, 0); > + while (TREE_CODE (expr) == COMPOUND_EXPR) > + expr = TREE_OPERAND (expr, 1); > + if (TREE_CODE (expr) == POINTER_PLUS_EXPR) > + expr = TREE_OPERAND (expr, 0); > + if (TREE_CODE (expr) == SAVE_EXPR) > + expr = TREE_OPERAND (expr, 0); > + STRIP_NOPS (expr); > + return expr; > + } > + > + return NULL_TREE; > +} > + > +static tree > +omp_containing_struct (tree expr) > +{ > + tree expr0 = expr; > + > + STRIP_NOPS (expr); > + > + tree expr1 = expr; > + > + /* FIXME: other types of accessors? */ > + while (TREE_CODE (expr) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + > + if (TREE_CODE (expr) == COMPONENT_REF) > + { > + if (DECL_P (TREE_OPERAND (expr, 0)) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == COMPONENT_REF > + || TREE_CODE (TREE_OPERAND (expr, 0)) == INDIRECT_REF > + || (TREE_CODE (TREE_OPERAND (expr, 0)) == MEM_REF > + && integer_zerop (TREE_OPERAND (TREE_OPERAND (expr, 0), 1))) > + || TREE_CODE (TREE_OPERAND (expr, 0)) == ARRAY_REF) > + expr = TREE_OPERAND (expr, 0); > + else > + internal_error ("unhandled component"); > + } Again? > @@ -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<omp_mapping_group> *groups; > + groups = omp_gather_mapping_groups (list_p); > + if (groups) > + { > + hash_map<tree_operand_hash, omp_mapping_group *> *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. > 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. Jakub