Re: [PATCH v2 03/11] OpenMP/OpenACC struct sibling list gimplification extension and rework

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 18, 2022 at 09:24:53AM -0700, Julian Brown wrote:
> 2022-03-17  Julian Brown  <julian@codesourcery.com>
> 
> 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 = 0x2000000,
>  
> -  /* Flag for GOVD_MAP: (struct) vars that have pointer attachments for
> -     fields.  */
> -  GOVD_MAP_HAS_ATTACHMENTS = 0x4000000,
> -
>    /* Flag for GOVD_FIRSTPRIVATE: OMP_CLAUSE_FIRSTPRIVATE_IMPLICIT.  */
>    GOVD_FIRSTPRIVATE_IMPLICIT = 0x8000000,

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 the previous node pointed to by INSERT_AT.  */
> +
> +static tree *
> +insert_node_after (tree newnode, tree *insert_at)
> +{
> +  OMP_CLAUSE_CHAIN (newnode) = *insert_at;
> +  *insert_at = newnode;
> +  return &OMP_CLAUSE_CHAIN (newnode);
> +}

Including these...  (etc.) The names are too generic for what they do.

> +static bool
> +omp_build_struct_sibling_lists (enum tree_code code,
> +				enum omp_region_type region_type,
> +				vec<omp_mapping_group> *groups,
> +				hash_map<tree_operand_hash, omp_mapping_group *>

Perhaps better wrap on the , align omp_mapping_group * below
tree_operand_hash and then **grpmap will fit.

> +				  **grpmap)

Otherwise LGTM.

	Jakub