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
[PATCH v2 04/11] OpenMP/OpenACC: Add inspector class to unify mapped address analysis
Several places in the C and C++ front-ends dig through OpenMP and OpenACC
addresses from "map" clauses (etc.) in order to determine whether they
are component accesses that need "attach" operations, check duplicate
mapping clauses, and so on. When we're extending support for more kinds
of lvalues in map clauses for OpenMP, it seems helpful to bring these
all into one place in order to keep all the analyses in sync, and to
make it easier to reason about which kinds of expressions are supported.
This patch introduces an "address inspector" class for that purpose,
and adjusts the C and C++ front-ends to use it.
Relative to the previous posted version, this patch heavily reworks the
internals of the "address inspector" class and its call sites in the
C and C++ front-ends in order to clarify the logic used to elaborate
"map" clause nodes, which had become somewhat convoluted. It also now
implements the functionality of the "c_omp_decompose_attachable_address"
function from earlier versions of this patch series.
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,
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.
gcc/c/
* c-typeck.c (handle_omp_array_sections_1,
handle_omp_array_sections, c_finish_omp_clauses): Use
c_omp_address_inspector class.
gcc/cp/
* semantics.c (cp_omp_address_inspector): New class, derived from
c_omp_address_inspector.
(handle_omp_array_sections_1, handle_omp_array_sections,
finish_omp_clauses): Use cp_omp_address_inspector class to
analyze OpenMP map clause expressions.
gcc/testsuite/
* g++.dg/gomp/unmappable-component-1.C: New test.
libgomp/
* testsuite/libgomp.c++/class-array-1.C: New test.
* testsuite/libgomp.c-c++-common/baseptrs-1.c: New test.
* testsuite/libgomp.c-c++-common/baseptrs-2.c: New test.
---
gcc/c-family/c-common.h | 55 +++
gcc/c-family/c-omp.cc | 268 +++
gcc/c/c-typeck.cc | 305 +---
gcc/cp/semantics.cc | 440 --
.../g++.dg/gomp/unmappable-component-1.C | 21 +
libgomp/testsuite/libgomp.c++/class-array-1.C | 59 +++
.../libgomp.c-c++-common/baseptrs-1.c | 50 ++
.../libgomp.c-c++-common/baseptrs-2.c | 70 +++
8 files changed, 814 insertions(+), 454 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/gomp/unmappable-component-1.C
create mode 100644 libgomp/testsuite/libgomp.c++/class-array-1.C
create mode 100644 libgomp/testsuite/libgomp.c-c++-common/baseptrs-1.c
create mode 100644 libgomp/testsuite/libgomp.c-c++-common/baseptrs-2.c
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index a8d6f82bb2c..e592e7fd368 100644
--- 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 ();
+}
+
+ bool map_supported_p ();
+
+ static tree get_origin (tree);
+ static tree peel_components (tree);
+ static tree maybe_peel_ref (tree);
+ static tree get_base_pointer (tree);
+ tree get_base_pointer () { return get_base_pointer (orig); }
+ static tree get_base_pointer_tgt (tree);
+ tree get_base_pointer_tgt () { return get_base_pointer_tgt
