Re: [PATCH v2 01/11] OpenMP 5.0: Clause ordering for OpenMP 5.0 (topological sorting by base pointer)
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.
> +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 *groups;
> + groups = omp_gather_mapping_group
Re: [PATCH v2 02/11] Remove omp_target_reorder_clauses
On Fri, Mar 18, 2022 at 09:24:52AM -0700, Julian Brown wrote: > This patch has been split out from the previous one to avoid a > confusingly-interleaved diff. The two patches should probably be > committed squashed together. Agreed, LGTM. > > 2021-10-01 Julian Brown > > gcc/ > * gimplify.c (omp_target_reorder_clauses): Delete. Jakub
Re: [PATCH v2 03/11] OpenMP/OpenACC struct sibling list gimplification extension and rework
On Fri, Mar 18, 2022 at 09:24:53AM -0700, Julian Brown wrote:
> 2022-03-17 Julian Brown
>
> gcc/fortran/
> * trans-openmp.cc (gfc_trans_omp_clauses): Don't create
> GOMP_MAP_TO_PSET mappings for class metadata, nor GOMP_MAP_POINTER
> mappings for POINTER_TYPE_P decls.
>
> gcc/
> * gimplify.c (gimplify_omp_var_data): Remove GOVD_MAP_HAS_ATTACHMENTS.
> (insert_struct_comp_map): Refactor function into...
> (build_struct_comp_nodes): This new function. Remove list handling
> and improve self-documentation.
> (extract_base_bit_offset): Remove BASE_REF, OFFSETP parameters. Move
> code to strip outer parts of address out of function, but strip no-op
> conversions.
> (omp_mapping_group): Add DELETED field for use during reindexing.
> (strip_components_and_deref, strip_indirections): New functions.
> (omp_group_last, omp_group_base): Add GOMP_MAP_STRUCT handling.
> (omp_gather_mapping_groups): Initialise DELETED field for new groups.
> (omp_index_mapping_groups): Notice DELETED groups when (re)indexing.
> (insert_node_after, move_node_after, move_nodes_after,
> move_concat_nodes_after): New helper functions.
> (accumulate_sibling_list): New function to build up GOMP_MAP_STRUCT
> node groups for sibling lists. Outlined from
> gimplify_scan_omp_clauses.
> (omp_build_struct_sibling_lists): New function.
> (gimplify_scan_omp_clauses): Remove struct_map_to_clause,
> struct_seen_clause, struct_deref_set. Call
> omp_build_struct_sibling_lists as pre-pass instead of handling sibling
> lists in the function's main processing loop.
> (gimplify_adjust_omp_clauses_1): Remove GOVD_MAP_HAS_ATTACHMENTS
> handling, unused now.
> * omp-low.cc (scan_sharing_clauses): Handle pointer-type indirect
> struct references, and references to pointers to structs also.
>
> gcc/testsuite/
> * g++.dg/goacc/member-array-acc.C: New test.
> * g++.dg/gomp/member-array-omp.C: New test.
> * g++.dg/gomp/target-3.C: Update expected output.
> * g++.dg/gomp/target-lambda-1.C: Likewise.
> * g++.dg/gomp/target-this-2.C: Likewise.
> * c-c++-common/goacc/deep-copy-arrayofstruct.c: Move test from here.
>
> libgomp/
> * testsuite/libgomp.oacc-c-c++-common/deep-copy-15.c: New test.
> * testsuite/libgomp.oacc-c-c++-common/deep-copy-16.c: New test.
> * testsuite/libgomp.oacc-c++/deep-copy-17.C: New test.
> * testsuite/libgomp.oacc-c-c++-common/deep-copy-arrayofstruct.c: Move
> test to here, make "run" test.
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -125,10 +125,6 @@ enum gimplify_omp_var_data
>/* Flag for GOVD_REDUCTION: inscan seen in {in,ex}clusive clause. */
>GOVD_REDUCTION_INSCAN = 0x200,
>
> - /* Flag for GOVD_MAP: (struct) vars that have pointer attachments for
> - fields. */
> - GOVD_MAP_HAS_ATTACHMENTS = 0x400,
> -
>/* Flag for GOVD_FIRSTPRIVATE: OMP_CLAUSE_FIRSTPRIVATE_IMPLICIT. */
>GOVD_FIRSTPRIVATE_IMPLICIT = 0x800,
I'd renumber the GOVD_* constants after this, otherwise we won't remember
we've left a gap.
> + (or derived type, etc.) component, create an "alloc" or "release" node to
> + insert into a list following a GOMP_MAP_STRUCT node. For some types of
> + mapping (e.g. Fortran arrays with descriptors), an additional mapping may
> + be created that is inserted into the list of mapping nodes attached to the
> + directive being processed -- not part of the sorted list of nodes after
> + GOMP_MAP_STRUCT.
> +
> + CODE is the code of the directive being processed. GRP_START and GRP_END
> + are the first and last of two or three nodes representing this array
> section
> + mapping (e.g. a data movement node like GOMP_MAP_{TO,FROM}, optionally a
> + GOMP_MAP_TO_PSET, and finally a GOMP_MAP_ALWAYS_POINTER). EXTRA_NODE is
> + filled with the additional node described above, if needed.
> +
> + This function does not add the new nodes to any lists itself. It is the
> + responsibility of the caller to do that. */
>
> 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 think it would be nice to use omp_ prefixes even for these static
functions, this is all in the gimplifier, so it should be clear that it
isn't some generic code but OpenMP specific gimplification code.
Another variant would be to introduce omp-gimplify.cc and move lots of stuff
there, but if we do that, best time might be during stage3 so that it
doesn't collide with too many patches.
>
> +/* Link node NEWNODE so it is pointed to by chain INSERT_AT. NEWNODE's chain
> + is linked to
Re: [PATCH v2 04/11] OpenMP/OpenACC: Add inspector class to unify mapped address analysis
On Fri, Mar 18, 2022 at 09:24:54AM -0700, Julian Brown wrote:
> 2022-03-17 Julian Brown
>
> gcc/c-family/
> * c-common.h (c_omp_address_inspector): New class.
> * c-omp.c (c_omp_address_inspector::get_deref_origin,
> c_omp_address_inspector::component_access_p,
> c_omp_address_inspector::check_clause,
> c_omp_address_inspector::get_root_term,
Spaces instead of tabs.
> c_omp_address_inspector::map_supported_p,
> c_omp_address_inspector::mappable_type,
> c_omp_address_inspector::get_origin,
> c_omp_address_inspector::peel_components,
> c_omp_address_inspector::maybe_peel_ref,
> c_omp_address_inspector::maybe_zero_length_array_section,
> c_omp_address_inspector::get_base_pointer,
> c_omp_address_inspector::get_base_pointer_tgt,
> c_omp_address_inspector::get_attachment_point): New methods.
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -1253,6 +1253,61 @@ extern void c_omp_mark_declare_variant (location_t,
> tree, tree);
> extern const char *c_omp_map_clause_name (tree, bool);
> extern void c_omp_adjust_map_clauses (tree, bool);
>
> +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 bool mappable_type (tree t);
> + virtual void emit_unmappable_type_notes (tree) { }
> +
> + bool check_clause (tree);
> + tree get_root_term (bool);
> +
> + tree get_address () { return orig; }
> + tree get_deref_origin ();
> + bool component_access_p ();
> +
> + bool has_indirections_p ()
> +{
> + if (!root_term)
> + get_root_term (false);
> + return indirections;
> +}
> +
> + bool indir_component_ref_p ()
> +{
> + return component_access_p () && has_indirections_p ();
> +}
I think https://gcc.gnu.org/codingconventions.html#Cxx_Conventions
just says that no member functions should be defined inside of the
class, which is something that almost nobody actually honors.
But, when they are inline, there should be one style, not many,
so either
type method (args)
{
}
(guess my preference) or
type method (args)
{
}
but not those mixed up, which you have in the patch.
> --- a/gcc/c-family/c-omp.cc
> +++ b/gcc/c-family/c-omp.cc
> @@ -3113,6 +3113,274 @@ c_omp_adjust_map_clauses (tree clauses, bool
> is_target)
> }
> }
>
There should be function comment for all the out of line definitions.
> +tree
> +c_omp_address_inspector::get_deref_origin ()
> {
>if (error_operand_p (t))
> return error_mark_node;
> + c_omp_address_inspector t_insp (OMP_CLAUSE_LOCATION (c), t);
Wouldn't ai (address inspector) be better than t_insp?
> +/* C++ specialisation of the c_omp_address_inspector class. */
> +
> +class cp_omp_address_inspector : public c_omp_address_inspector
> +{
> +public:
> + cp_omp_address_inspector (location_t loc, tree t)
> +: c_omp_address_inspector (loc, t)
> + { }
> +
> + ~cp_omp_address_inspector ()
> + { }
> +
> + bool processing_template_decl_p ()
> + {
> +return processing_template_decl;
> + }
> +
> + bool mappable_type (tree t)
> + {
> +return cp_omp_mappable_type (t);
> + }
> +
> + void emit_unmappable_type_notes (tree t)
> + {
> +cp_omp_emit_unmappable_type_notes (t);
> + }
> +
> + static bool ref_p (tree t)
> +{
> + return (TYPE_REF_P (TREE_TYPE (t))
> + || REFERENCE_REF_P (t));
> +}
See above the mixing of styles...
I know, some headers are really bad examples, e.g. hash-map.h
even has 3 different styles,
{
}
and
{
}
and
{
}
for the type method (args) indented by 2 spaces.
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/gomp/unmappable-component-1.C
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +
> +struct A {
> + static int x[10];
> +};
> +
> +struct B {
> + A a;
> +};
> +
> +int
> +main (int argc, char *argv[])
> +{
> + B *b = new B;
> +#pragma omp target map(b->a) // { dg-error "'b->B::a' does not have a
> mappable type in 'map' clause" }
> + ;
> + B bb;
> +#pragma omp target map(bb.a) // { dg-error "'bb\.B::a' does not have a
> mappable type in 'map' clause" }
We don't diagnose static data members as non-mappable anymore.
So I don't see how this test can work.
> +int
> +main (int argc, char *argv[])
Why "int argc, char *argv[]" when you don't use it?
> + p0 = (p0type *) malloc (sizeof *p0);
> + p0->x0[0].p1 = (p1type *) malloc (sizeof *p0->x0[0].p1);
> + p0->x0[0].p1->p2 = (p2type *) malloc (sizeof *p0->x0[0].p1->p2);
> + memset (p0->x0[0].p1->p2, 0, sizeof *p0->x0[0].p1->p2);
> +
> +#pragma omp target map(
Re: [PATCH v2 05/11] OpenMP: Handle reference-typed struct members
On Fri, Mar 18, 2022 at 09:26:46AM -0700, Julian Brown wrote: > This patch relates to OpenMP mapping clauses containing struct members of > reference type, e.g. "mystruct.myref.myptr[:N]". To be able to access > the array slice through the reference in the middle, we need to perform > an attach action for that reference, since it is represented internally > as a pointer. > > I don't think the spec allows for this case explicitly. The closest > clause is (OpenMP 5.0, "2.19.7.1 map Clause"): > > "If the type of a list item is a reference to a type T then the >reference in the device data environment is initialized to refer to >the object in the device data environment that corresponds to the >object referenced by the list item. If mapping occurs, it occurs as >though the object were mapped through a pointer with an array section >of type T and length one." Plus the general rule that aggregates are mapped as mapping of all its individual members/elements. > --- a/gcc/gimplify.cc > +++ b/gcc/gimplify.cc > @@ -9813,7 +9813,10 @@ accumulate_sibling_list (enum omp_region_type > region_type, enum tree_code code, >/* FIXME: If we're not mapping the base pointer in some other clause on > this > directive, I think we want to create ALLOC/RELEASE here -- i.e. not > early-exit. */ > - if (openmp && attach_detach) > + if (openmp > + && attach_detach > + && !(TREE_CODE (TREE_TYPE (ocd)) == REFERENCE_TYPE > +&& TREE_CODE (TREE_TYPE (TREE_TYPE (ocd))) != POINTER_TYPE)) > return NULL; Why isn't a reference to pointer handled that way too? Jakub
Re: [PATCH v2 06/11] OpenMP: lvalue parsing for map clauses (C++)
On Fri, Mar 18, 2022 at 09:26:47AM -0700, Julian Brown wrote:
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -4266,6 +4266,9 @@ cp_parser_new (cp_lexer *lexer)
>parser->omp_declare_simd = NULL;
>parser->oacc_routine = NULL;
>
> + /* Allow array slice in expression. */
Better /* Disallow OpenMP array sections in expressions. */
> + parser->omp_array_section_p = false;
> +
>/* Not declaring an implicit function template. */
>parser->auto_is_implicit_function_template_parm_p = false;
>parser->fully_implicit_function_template_p = false;
I think we should figure out when we should temporarily disable
parser->omp_array_section_p = false;
and restore it afterwards to a saved value. E.g.
cp_parser_lambda_expression seems like a good candidate, the fact that
OpenMP array sections are allowed say in map clause doesn't mean they are
allowed inside of lambdas and it would be especially hard when the lambda
is defining a separate function and the search for OMP_ARRAY_SECTION
probably wouldn't be able to discover those.
Other spots to consider might be statement expressions, perhaps type
definitions etc.
> @@ -8021,6 +8024,7 @@ cp_parser_postfix_open_square_expression (cp_parser
> *parser,
>releasing_vec expression_list = NULL;
>location_t loc = cp_lexer_peek_token (parser->lexer)->location;
>bool saved_greater_than_is_operator_p;
> + bool saved_colon_corrects_to_scope_p;
>
>/* Consume the `[' token. */
>cp_lexer_consume_token (parser->lexer);
> @@ -8028,6 +8032,9 @@ cp_parser_postfix_open_square_expression (cp_parser
> *parser,
>saved_greater_than_is_operator_p = parser->greater_than_is_operator_p;
>parser->greater_than_is_operator_p = true;
>
> + saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
> + parser->colon_corrects_to_scope_p = false;
I think the last above line should be guarded on
if (parser->omp_array_section_p)
There is no reason to get worse diagnostics in non-OpenMP code or even in
OpenMP code where array sections aren't allowed.
> +
> + /* NOTE: We are reusing using the type of the whole array as the type
> of
> + the array section here, which isn't necessarily entirely correct.
> + Might need revisiting. */
"reusing using" looks weird.
As for the type of OMP_ARRAY_SECTION trees, perhaps we could initially use
an incomplete array (so array element would be meaningful)
and when we figure out the details and the array section is contiguous
change its type to array type covering it.
> + return build3_loc (input_location, OMP_ARRAY_SECTION,
> + TREE_TYPE (postfix_expression),
> + postfix_expression, index, length);
> +}
> +
> + parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
> +
>/* Look for the closing `]'. */
>cp_parser_require (parser, CPP_CLOSE_SQUARE, RT_CLOSE_SQUARE);
>
> @@ -36536,7 +36570,7 @@ struct omp_dim
> static tree
> cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
> tree list, bool *colon,
> - bool allow_deref = false)
> + bool map_lvalue = false)
> {
>auto_vec dims;
>bool array_section_p;
> @@ -36547,12 +36581,95 @@ cp_parser_omp_var_list_no_open (cp_parser *parser,
> enum omp_clause_code kind,
>parser->colon_corrects_to_scope_p = false;
>*colon = false;
> }
> + begin_scope (sk_omp, NULL);
Why? Base-language-wise, clauses don't introduce a new scope
for name-lookup.
And if it is really needed, I'd strongly prefer to either do it solely
for the clauses that might need it, or do begin_scope before first
such clause and finish at the end if it has been introduced.
>while (1)
> {
>tree name, decl;
>
>if (kind == OMP_CLAUSE_DEPEND || kind == OMP_CLAUSE_AFFINITY)
> cp_parser_parse_tentatively (parser);
> + else if (map_lvalue && kind == OMP_CLAUSE_MAP)
> + {
This shouldn't be done just for OMP_CLAUSE_MAP, but for all the
other clauses that accept array sections, including
OMP_CLAUSE_DEPEND, OMP_CLAUSE_AFFINITY, OMP_CLAUSE_MAP, OMP_CLAUSE_TO,
OMP_CLAUSE_FROM, OMP_CLAUSE_INCLUSIVE, OMP_CLAUSE_EXCLUSIVE,
OMP_CLAUSE_USE_DEVICE_ADDR, OMP_CLAUSE_HAS_DEVICE_ADDR,
OMP_CLAUSE_*REDUCTION.
And preferrably, they should be kept in the IL until *finish_omp_clauses,
which should handle those instead of TREE_LIST that represented them before.
Additionally, something should diagnose incorrect uses of OMP_ARRAY_SECTION,
which is everywhere in the expressions but as the outermost node(s),
i.e. for clauses that do allow array sections scan OMP_CLAUSE_DECL after
handling handleable array sections and complain about embedded
OMP_ARRAY_SECTION, including OMP_ARRAY_SECTION say in the lower-bound,
length and/or stride expressions of the valid OMP_ARRAY_SECTION.
For C++ that also means handling OMP_ARRAY_SECTION code in pt.c.
Re: [PATCH v2 08/11] Use OMP_ARRAY_SECTION instead of TREE_LIST in C++ FE
On Fri, Mar 18, 2022 at 09:26:49AM -0700, Julian Brown wrote: > This patch changes the representation of OMP array sections in the > C++ front end to use the new OMP_ARRAY_SECTION tree code instead of a > TREE_LIST. This is important for "declare mapper" support, because the > array section representation may stick around longer (in "declare mapper" > definitions), and special-case handling TREE_LIST becomes necessary in > more places, which starts to become unwieldy. > > 2022-02-18 Julian Brown > > gcc/c-family/ > * c-omp.cc (c_omp_split_clauses): Support OMP_ARRAY_SECTION. > > gcc/cp/ > * parser.cc (cp_parser_omp_var_list_no_open): Use OMP_ARRAY_SECTION > code instead of TREE_LIST to represent OpenMP array sections. > * pt.cc (tsubst_copy, tsubst_omp_clause_decl, tsubst_copy_and_build): > Add OMP_ARRAY_SECTION support. > * semantics.cc (handle_omp_array_sections_1, handle_omp_array_sections, > cp_oacc_check_attachments, finish_omp_clauses): Use OMP_ARRAY_SECTION > instead of TREE_LIST where appropriate. > * gimplify.cc (gimplify_expr): Ensure OMP_ARRAY_SECTION has been > processed out before gimplification. THis is all a step towards the right direction, but we really do want to transition from uses of TREE_LIST to represent array sections to OMP_ARRAY_SECTION. For some clauses that do allow lvalue expressions that is a must, for the rest just a good cleanup even when the OMP_ARRAY_SECTION are created instead of TREE_LIST during the explicit array section parsing in OpenMP var list parsing. Jakub
Re: [PATCH v2 09/11] OpenMP 5.0 "declare mapper" support for C++
On Fri, Mar 18, 2022 at 09:26:50AM -0700, Julian Brown wrote:
> This patch implements OpenMP 5.0 "declare mapper" support for C++ --
> except for arrays of structs with mappers, which are TBD. I've taken cues
> from the existing "declare reduction" support where appropriate, though
> obviously the details of implementation differ somewhat (in particular,
> "declare mappers" must survive longer, until gimplification time).
>
> Both named and unnamed (default) mappers are supported, and both
> explicitly-mapped structures and implicitly-mapped struct-typed variables
> used within an offload region are supported. For the latter, we need a
> way to communicate to the middle end which mapper (visible, in-scope) is
> the right one to use -- for that, we scan the target body in the front
> end for uses of structure (etc.) types, and create artificial "mapper
> binding" clauses to associate types with visible mappers. (It doesn't
> matter too much if we create these mapper bindings a bit over-eagerly,
> since they will only be used if needed later during gimplification.)
>
> Another difficulty concerns the order in which an OpenMP offload region
> body's clauses are processed relative to its body: in order to add
> clauses for instantiated mappers, we need to have processed the body
> already in order to find which variables have been used, but at present
> the region's clauses are processed strictly before the body. So, this
> patch adds a second clause-processing step after gimplification of the
> body in order to add clauses resulting from instantiated mappers.
>
> This version of the patch improves detection of explicitly-mapped struct
> accesses which inhibit implicitly-triggered user-defined mappers for a
> target region.
Will start with a general comment, from looking at the dumps it seems
handling the mappers in the FE right away for explicit mapping clauses
and attaching mapper binding clauses for types that are (or could
conservatively be, including from the recursive mappers themselves) be
used in the target body and letting gimplification find those var in detail
and use mapper binding clauses to actually expand it looks like the right
approach to me. As I raised in an earlier patch, a big question is if we
should do map clause sorting on gimplify_scan_omp_clauses or
gimplify_adjust_omp_clauses or both...
The conservative discovery of what types we might need to create mapper
binding clauses for should be probably done only if
!processing_template_decl.
One question is though if DECL_OMP_DECLARE_MAPPER_P should be a magic
FUNCTION_DECL or a magic TREE_STATIC VAR_DECL or say CONST_DECLs.
The reason for the choice of FUNCTION_DECLs for UDRs is that they actually
contain code, but for UDMs we don't need any code, all we need is some
decl to which we can somehow attach list of clauses and a placeholder
decl used in them. Perhaps a magic VAR_DECL or CONST_DECL would be
cheaper than a FUNCTION_DECL...
> @@ -32193,6 +32197,16 @@ cp_parser_late_parsing_for_member (cp_parser*
> parser, tree member_function)
> finish_function (/*inline_p=*/true);
> cp_check_omp_declare_reduction (member_function);
> }
> + else if (DECL_OMP_DECLARE_MAPPER_P (member_function))
> + {
> + parser->lexer->in_pragma = true;
> + cp_parser_omp_declare_mapper_maplist (member_function, parser);
> + finish_function (/*inline_p=*/true);
> + cp_check_omp_declare_mapper (member_function);
> + /* If this is a template class, this forces the body of the mapper
> + to be instantiated. */
> + DECL_PRESERVE_P (member_function) = 1;
UDRs don't do this. Why aren't the clauses instantiated when we actually
need such a template?
> @@ -39509,11 +39522,27 @@ cp_parser_omp_clause_map (cp_parser *parser, tree
> list)
>
>if (cp_lexer_peek_nth_token (parser->lexer, pos + 1)->type ==
> CPP_COMMA)
> pos++;
> + else if ((cp_lexer_peek_nth_token (parser->lexer, pos + 1)->type
> + == CPP_OPEN_PAREN)
> +&& ((cp_lexer_peek_nth_token (parser->lexer, pos + 2)->type
> + == CPP_NAME)
> +|| ((cp_lexer_peek_nth_token (parser->lexer, pos + 2)->type
> + == CPP_KEYWORD)
> +&& (cp_lexer_peek_nth_token (parser->lexer,
> + pos + 2)->keyword
> +== RID_DEFAULT)))
> +&& (cp_lexer_peek_nth_token (parser->lexer, pos + 3)->type
> +== CPP_CLOSE_PAREN)
> +&& (cp_lexer_peek_nth_token (parser->lexer, pos + 4)->type
> +== CPP_COMMA))
In this loop we don't need to be exact, all we want is find out
if the mapper-mdifier candidates are followed by : or not, the
actual parsing is done only later. So, can't we just use
for CPP_OPEN_PAREN cp_parser_skip_balanced_tokens to move over
all the modifier's arguments?
Jakub
