Re: [PATCH v3 08/11] OpenMP/OpenACC: Rework clause expansion and nested struct handling

[Julian Brown <julian@codesourcery.com>]

On Wed, 14 Sep 2022 15:24:12 +0200
Jakub Jelinek via Fortran <fortran@gcc.gnu.org> 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<tree_operand_hash,
> > +					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_node);
> > +  splay_tree_key n2 = gomp_map_0len_lookup (mem_map, &cur_node);
> >    kind = get_kind (short_mapkind, kinds, i);
> >    implicit = get_implicit (short_mapkind, kinds, i);
> >    if (n2  
> 
> I'm a little bit worried if the above isn't a backwards incompatible
> change. We need programs compiled by GCC 12 and earlier to be handled
> correctly.

I think that should be fine for that change -- zero-length fields won't
be present for GCC 12-compiled code, and if they were, it'd be a
runtime crash.

> > --- /dev/null
> > +++ b/libgomp/testsuite/libgomp.c++/baseptrs-4.C
> > @@ -0,0 +1,3154 @@
> > +// { dg-do run }
> > +
> > +#include <cstring>
> > +#include <cassert>
> > +
> > +#define MAP_DECLS
> > +
> > +#define NONREF_DECL_BASE
> > +#define REF_DECL_BASE
> > +#define PTR_DECL_BASE
> > +#define REF2PTR_DECL_BASE
> > +
> > +#define ARRAY_DECL_BASE
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2ARRAY_DECL_BASE
> > +#define PTR_OFFSET_DECL_BASE
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2PTR_OFFSET_DECL_BASE
> > +
> > +#define MAP_SECTIONS
> > +
> > +#define NONREF_DECL_MEMBER_SLICE
> > +#define NONREF_DECL_MEMBER_SLICE_BASEPTR
> > +#define REF_DECL_MEMBER_SLICE
> > +#define REF_DECL_MEMBER_SLICE_BASEPTR
> > +#define PTR_DECL_MEMBER_SLICE
> > +#define PTR_DECL_MEMBER_SLICE_BASEPTR
> > +#define REF2PTR_DECL_MEMBER_SLICE
> > +#define REF2PTR_DECL_MEMBER_SLICE_BASEPTR
> > +
> > +#define ARRAY_DECL_MEMBER_SLICE
> > +#define ARRAY_DECL_MEMBER_SLICE_BASEPTR
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2ARRAY_DECL_MEMBER_SLICE
> > +//#define REF2ARRAY_DECL_MEMBER_SLICE_BASEPTR
> > +#define PTR_OFFSET_DECL_MEMBER_SLICE
> > +#define PTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2PTR_OFFSET_DECL_MEMBER_SLICE
> > +//#define REF2PTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR
> > +
> > +#define PTRARRAY_DECL_MEMBER_SLICE
> > +#define PTRARRAY_DECL_MEMBER_SLICE_BASEPTR
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2PTRARRAY_DECL_MEMBER_SLICE
> > +//#define REF2PTRARRAY_DECL_MEMBER_SLICE_BASEPTR
> > +#define PTRPTR_OFFSET_DECL_MEMBER_SLICE
> > +#define PTRPTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR
> > +// Needs map clause "lvalue"-parsing support.
> > +//#define REF2PTRPTR_OFFSET_DECL_MEMBER_SLICE
> > +//#define REF2PTRPTR_OFFSET_DECL_MEMBER_SLICE_BASEPTR
> > +
> > +#define NONREF_COMPONENT_BASE
> > +#define NONREF_COMPONENT_MEMBER_SLICE
> > +#define NONREF_COMPONENT_MEMBER_SLICE_BASEPTR
> > +
> > +#define REF_COMPONENT_BASE
> > +#define REF_COMPONENT_MEMBER_SLICE
> > +#define REF_COMPONENT_MEMBER_SLICE_BASEPTR
> > +
> > +#define PTR_COMPONENT_BASE
> > +#define PTR_COMPONENT_MEMBER_SLICE
> > +#define PTR_COMPONENT_MEMBER_SLICE_BASEPTR
> > +
> > +#define REF2PTR_COMPONENT_BASE
> > +#define REF2PTR_COMPONENT_MEMBER_SLICE
> > +#define REF2PTR_COMPONENT_MEMBER_SLICE_BASEPTR  
> 
> Are all these defines only a temporary hacks until the patchset
> is complete?

Yes -- those commented-out bits are re-enabled by the following patch
("FYI/unfinished: OpenMP: lvalue parsing for map   clauses (C++)"),
though that one still has previous review comments to be addressed also.

Thanks,

Julian