Re: [PATCH v2 04/11] OpenMP/OpenACC: Add inspector class to unify mapped address analysis

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Mar 18, 2022 at 09:24:54AM -0700, Julian Brown wrote:
> 2022-03-17  Julian Brown  <julian@codesourcery.com>
> 
> 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(tofrom: p0->x0[k1].p1->p2[k2].x1[k3].x2[4][0:n]) \
> +		   map(to: p0->x0[k1].p1, p0->x0[k1].p1->p2) \
> +		   map(to: p0->x0[k1].p1[0])
> +  {
> +    for (int i = 0; i < n; i++)
> +      p0->x0[k1].p1->p2[k2].x1[k3].x2[4][i] = i;
> +  }
> +
> +  for (int i = 0; i < n; i++)
> +    assert (i == p0->x0[k1].p1->p2[k2].x1[k3].x2[4][i]);

It would be nice to free pointers afterwards in order not to give bad coding
examples to people.

Otherwise LGTM.

	Jakub