Hi! I'm sending this again with some more people copied -- because I see you're working on tree.h/tree-core.h flattening, or know you're familiar with GCC plugins. ;-) Here is a question concerning both of that, where I'd appreciate your input.
(That said, I don't find a "GCC plugins" person listed in the MAINTAINERS
file, would that be worth adding?)
Full quote follows:
On Fri, 19 Dec 2014 18:54:04 +0100, I wrote:
> On Thu, 18 Dec 2014 19:33:07 +0100, Jakub Jelinek <ja...@redhat.com> wrote:
> > On Thu, Dec 18, 2014 at 07:25:03PM +0100, Thomas Schwinge wrote:
> > > On Wed, 17 Dec 2014 23:26:53 +0100, I wrote:
> > > > Committed to gomp-4_0-branch in r218840:
> > > >
> > > > commit febcd8dfdb10fa80edff0880973d1915ca2fef74
> > > > Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
> > > > Date: Wed Dec 17 22:26:24 2014 +0000
> > > >
> > > > Use include/gomp-constants.h more actively.
> > >
> > > > diff --git gcc/tree-core.h gcc/tree-core.h
> > > > index 743bc0d..fc61b88 100644
> > > > --- gcc/tree-core.h
> > > > +++ gcc/tree-core.h
> > > > @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see
> > > > #include "alias.h"
> > > > #include "flags.h"
> > > > #include "symtab.h"
> > > > +#include "gomp-constants.h"
> > > >
> > > > /* This file contains all the data structures that define the 'tree'
> > > > type.
> > > > There are no accessor macros nor functions in this file. Only the
> > >
> > > Is it actually "OK" to #include "gomp-constants.h" (living in
> > > [GCC]/include/) from gcc/tree-core.h? Isn't the tree-core.h file getting
> > > installed, for later use by plugins -- which then need to be able to find
> > > gomp-constants, too, but that is not being installed?
> >
> > Generally, it must be possible to include include/ headers from gcc/
> > headers, even when they are used by plugins. Otherwise system.h including
> > libiberty.h and safe-ctype.h etc. wouldn't work. Perhaps you need to add
> > gomp-constants.h to some Makefile variable or something, look at how is
> > safe-ctype.h etc. handled.
>
> Aha, that's how it is done, I guess, in gcc/Makefile.in:
>
> [...]
> SYSTEM_H = system.h hwint.h $(srcdir)/../include/libiberty.h \
> $(srcdir)/../include/safe-ctype.h $(srcdir)/../include/filenames.h
> [...]
> PLUGIN_HEADERS = $(TREE_H) $(CONFIG_H) $(SYSTEM_H) coretypes.h [...]
> [...]
> # Install the headers needed to build a plugin.
> install-plugin: installdirs lang.install-plugin s-header-vars
> install-gengtype
> # We keep the directory structure for files in config or c-family and .def
> # files. All other files are flattened to a single directory.
> $(mkinstalldirs) $(DESTDIR)$(plugin_includedir)
> headers=`echo $(PLUGIN_HEADERS) | tr ' ' '\012' | sort -u`; \
> [...]
> [...]
>
> > That said, including gomp-constants.h from tree-core.h is I think very much
> > against all the Andrew's efforts to flatten headers (which is something I'm
> > not very happy with generally, but in this case, I think only the very few
> > files that really need the constants should include it).
>
> Like this (not yet applied)?
[Jakub: »I think it is fine.«]
> Talking about external code (GCC plugins), do we have to take any
> measures about the removed enum omp_clause_map_kind? (Would a mere
> »#define omp_clause_map_kind gomp_map_kind« work? That'd also mean that
> we do have to add include/gomp-constants.h to PLUGIN_HEADERS, and get it
> included automatically, I think?)
>
> commit b1255597c6b069719960e53e385399c479c4be8b
> Author: Thomas Schwinge <tho...@codesourcery.com>
> Date: Fri Dec 19 18:32:25 2014 +0100
>
> Replace enum omp_clause_map_kind with enum gomp_map_kind.
>
> gcc/
> * tree-core.h: Instead of defining enum omp_clause_map_kind, use
> include/gomp-constants.h's enum gomp_map_kind. Update all users.
> include/
> * gomp-constants.h: Populate enum gomp_map_kind.
> ---
> gcc/c/c-parser.c | 38 ++++++++++++++---------------
> gcc/c/c-typeck.c | 9 +++----
> gcc/cp/parser.c | 38 ++++++++++++++---------------
> gcc/cp/semantics.c | 9 +++----
> gcc/fortran/trans-openmp.c | 47 ++++++++++++++++++------------------
> gcc/gimplify.c | 18 +++++++-------
> gcc/omp-low.c | 60
> ++++++++++++++++++++++------------------------
> gcc/tree-core.h | 43 +++------------------------------
> gcc/tree-nested.c | 8 +++----
> gcc/tree-pretty-print.c | 31 ++++++++++++------------
> gcc/tree-streamer-in.c | 2 +-
> gcc/tree-streamer-out.c | 2 +-
> gcc/tree.h | 4 ++--
> include/gomp-constants.h | 50 +++++++++++++++++++++++++++-----------
> 14 files changed, 173 insertions(+), 186 deletions(-)
Here is (this is on top of gomp-4_0-branch, by the way) the patch:
reordererd, and snipped to relevant parts.
The new "provider":
> --- include/gomp-constants.h
> +++ include/gomp-constants.h
> @@ -32,7 +32,7 @@
> /* Memory mapping types. */
>
> /* One byte. */
> -#define GOMP_MAP_VALUE_LIMIT (1 << 8)
> +#define GOMP_MAP_LAST (1 << 8)
>
> #define GOMP_MAP_FLAG_TO (1 << 0)
> #define GOMP_MAP_FLAG_FROM (1 << 1)
> @@ -44,19 +44,41 @@
> /* Flag to force a specific behavior (or else, trigger a run-time error). */
> #define GOMP_MAP_FLAG_FORCE (1 << 7)
>
> -#define GOMP_MAP_ALLOC 0
> -#define GOMP_MAP_TO (GOMP_MAP_ALLOC | GOMP_MAP_FLAG_TO)
> -#define GOMP_MAP_FROM (GOMP_MAP_ALLOC |
> GOMP_MAP_FLAG_FROM)
> -#define GOMP_MAP_TOFROM (GOMP_MAP_TO | GOMP_MAP_FROM)
> -#define GOMP_MAP_POINTER (GOMP_MAP_FLAG_SPECIAL_0 | 0)
> -#define GOMP_MAP_TO_PSET (GOMP_MAP_FLAG_SPECIAL_0 | 1)
> -#define GOMP_MAP_FORCE_PRESENT (GOMP_MAP_FLAG_SPECIAL_0 | 2)
> -#define GOMP_MAP_FORCE_DEALLOC (GOMP_MAP_FLAG_SPECIAL_0 | 3)
> -#define GOMP_MAP_FORCE_DEVICEPTR (GOMP_MAP_FLAG_SPECIAL_1 | 0)
> -#define GOMP_MAP_FORCE_ALLOC (GOMP_MAP_FLAG_FORCE | GOMP_MAP_ALLOC)
> -#define GOMP_MAP_FORCE_TO (GOMP_MAP_FLAG_FORCE | GOMP_MAP_TO)
> -#define GOMP_MAP_FORCE_FROM (GOMP_MAP_FLAG_FORCE | GOMP_MAP_FROM)
> -#define GOMP_MAP_FORCE_TOFROM (GOMP_MAP_FLAG_FORCE |
> GOMP_MAP_TOFROM)
> +enum gomp_map_kind
> + {
> + /* If not already present, allocate. */
> + GOMP_MAP_ALLOC = 0,
> + /* ..., and copy to device. */
> + GOMP_MAP_TO = (GOMP_MAP_ALLOC | GOMP_MAP_FLAG_TO),
> + /* ..., and copy from device. */
> + GOMP_MAP_FROM = (GOMP_MAP_ALLOC | GOMP_MAP_FLAG_FROM),
> + /* ..., and copy to and from device. */
> + GOMP_MAP_TOFROM = (GOMP_MAP_TO | GOMP_MAP_FROM),
> + /* The following kind is an internal only map kind, used for pointer
> based
> + array sections. OMP_CLAUSE_SIZE for these is not the pointer size,
> + which is implicitly POINTER_SIZE_UNITS, but the bias. */
> + GOMP_MAP_POINTER = (GOMP_MAP_FLAG_SPECIAL_0 | 0),
> + /* Also internal, behaves like GOMP_MAP_TO, but additionally any
> + GOMP_MAP_POINTER records consecutive after it which have addresses
> + falling into that range will not be ignored if GOMP_MAP_TO_PSET wasn't
> + mapped already. */
> + GOMP_MAP_TO_PSET = (GOMP_MAP_FLAG_SPECIAL_0 | 1),
> + /* Must already be present. */
> + GOMP_MAP_FORCE_PRESENT = (GOMP_MAP_FLAG_SPECIAL_0 | 2),
> + /* Deallocate a mapping, without copying from device. */
> + GOMP_MAP_FORCE_DEALLOC = (GOMP_MAP_FLAG_SPECIAL_0 | 3),
> + /* Is a device pointer. OMP_CLAUSE_SIZE for these is unused; is
> implicitly
> + POINTER_SIZE_UNITS. */
> + GOMP_MAP_FORCE_DEVICEPTR = (GOMP_MAP_FLAG_SPECIAL_1 | 0),
> + /* Allocate. */
> + GOMP_MAP_FORCE_ALLOC = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_ALLOC),
> + /* ..., and copy to device. */
> + GOMP_MAP_FORCE_TO = (GOMP_MAP_FLAG_FORCE |
> GOMP_MAP_TO),
> + /* ..., and copy from device. */
> + GOMP_MAP_FORCE_FROM = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_FROM),
> + /* ..., and copy to and from device. */
> + GOMP_MAP_FORCE_TOFROM = (GOMP_MAP_FLAG_FORCE | GOMP_MAP_TOFROM)
> + };
>
> #define GOMP_MAP_COPY_TO_P(X) \
> (!((X) & GOMP_MAP_FLAG_SPECIAL) \
The former "provider":
> --- gcc/tree-core.h
> +++ gcc/tree-core.h
> @@ -1239,45 +1239,8 @@ enum omp_clause_depend_kind
> OMP_CLAUSE_DEPEND_LAST
> };
>
> -enum omp_clause_map_kind
> -{
> - /* If not already present, allocate. */
> - OMP_CLAUSE_MAP_ALLOC = GOMP_MAP_ALLOC,
> - /* ..., and copy to device. */
> - OMP_CLAUSE_MAP_TO = GOMP_MAP_TO,
> - /* ..., and copy from device. */
> - OMP_CLAUSE_MAP_FROM = GOMP_MAP_FROM,
> - /* ..., and copy to and from device. */
> - OMP_CLAUSE_MAP_TOFROM = GOMP_MAP_TOFROM,
> - /* The following kind is an internal only map kind, used for pointer based
> - array sections. OMP_CLAUSE_SIZE for these is not the pointer size,
> - which is implicitly POINTER_SIZE_UNITS, but the bias. */
> - OMP_CLAUSE_MAP_POINTER = GOMP_MAP_POINTER,
> - /* Also internal, behaves like OMP_CLAUS_MAP_TO, but additionally any
> - OMP_CLAUSE_MAP_POINTER records consecutive after it which have addresses
> - falling into that range will not be ignored if OMP_CLAUSE_MAP_TO_PSET
> - wasn't mapped already. */
> - OMP_CLAUSE_MAP_TO_PSET = GOMP_MAP_TO_PSET,
> - /* The following are only valid for OpenACC. */
> - /* Allocate. */
> - OMP_CLAUSE_MAP_FORCE_ALLOC = GOMP_MAP_FORCE_ALLOC,
> - /* ..., and copy to device. */
> - OMP_CLAUSE_MAP_FORCE_TO = GOMP_MAP_FORCE_TO,
> - /* ..., and copy from device. */
> - OMP_CLAUSE_MAP_FORCE_FROM = GOMP_MAP_FORCE_FROM,
> - /* ..., and copy to and from device. */
> - OMP_CLAUSE_MAP_FORCE_TOFROM = GOMP_MAP_FORCE_TOFROM,
> - /* Must already be present. */
> - OMP_CLAUSE_MAP_FORCE_PRESENT = GOMP_MAP_FORCE_PRESENT,
> - /* Deallocate a mapping, without copying from device. */
> - OMP_CLAUSE_MAP_FORCE_DEALLOC = GOMP_MAP_FORCE_DEALLOC,
> - /* Is a device pointer. OMP_CLAUSE_SIZE for these is unused; is implicitly
> - POINTER_SIZE_UNITS. */
> - OMP_CLAUSE_MAP_FORCE_DEVICEPTR = GOMP_MAP_FORCE_DEVICEPTR,
> -
> - /* End marker. */
> - OMP_CLAUSE_MAP_LAST = GOMP_MAP_VALUE_LIMIT
> -};
> +/* See include/gomp-constants.h. */
> +enum gomp_map_kind;
>
> enum omp_clause_proc_bind_kind
> {
> @@ -1350,7 +1313,7 @@ struct GTY(()) tree_omp_clause {
> enum omp_clause_default_kind default_kind;
> enum omp_clause_schedule_kind schedule_kind;
> enum omp_clause_depend_kind depend_kind;
> - enum omp_clause_map_kind map_kind;
> + enum gomp_map_kind map_kind;
> enum omp_clause_proc_bind_kind proc_bind_kind;
> enum tree_code reduction_code;
> } GTY ((skip)) subcode;
A sample of the "consumers":
> --- gcc/c/c-parser.c
> +++ gcc/c/c-parser.c
> [...]
> @@ -11372,7 +11372,7 @@ static tree
> c_parser_omp_clause_map (c_parser *parser, tree list)
> {
> location_t clause_loc = c_parser_peek_token (parser)->location;
> - enum omp_clause_map_kind kind = OMP_CLAUSE_MAP_TOFROM;
> + enum gomp_map_kind kind = GOMP_MAP_TOFROM;
> tree nl, c;
>
> if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> @@ -11383,13 +11383,13 @@ c_parser_omp_clause_map (c_parser *parser, tree
> list)
> {
> const char *p = IDENTIFIER_POINTER (c_parser_peek_token
> (parser)->value);
> if (strcmp ("alloc", p) == 0)
> - kind = OMP_CLAUSE_MAP_ALLOC;
> + kind = GOMP_MAP_ALLOC;
> [...]
> --- gcc/gimplify.c
> +++ gcc/gimplify.c
> @@ -70,6 +70,7 @@ along with GCC; see the file COPYING3. If not see
> #include "omp-low.h"
> #include "gimple-low.h"
> #include "cilk.h"
> +#include "gomp-constants.h"
>
> #include "langhooks-def.h" /* FIXME: for lhd_set_decl_assembler_name */
> #include "tree-pass.h" /* FIXME: only for PROP_gimple_any */
> @@ -6436,8 +6437,8 @@ gimplify_adjust_omp_clauses_1 (splay_tree_node n, void
> *data)
> else if (code == OMP_CLAUSE_MAP)
> {
> OMP_CLAUSE_MAP_KIND (clause) = flags & GOVD_MAP_TO_ONLY
> - ? OMP_CLAUSE_MAP_TO
> - : OMP_CLAUSE_MAP_TOFROM;
> + ? GOMP_MAP_TO
> + : GOMP_MAP_TOFROM;
> [...]
> --- gcc/tree-streamer-in.c
> +++ gcc/tree-streamer-in.c
> @@ -427,7 +427,7 @@ unpack_ts_omp_clause_value_fields (struct data_in
> *data_in,
> break;
> case OMP_CLAUSE_MAP:
> OMP_CLAUSE_MAP_KIND (expr)
> - = bp_unpack_enum (bp, omp_clause_map_kind, OMP_CLAUSE_MAP_LAST);
> + = bp_unpack_enum (bp, gomp_map_kind, GOMP_MAP_LAST);
> break;
> case OMP_CLAUSE_PROC_BIND:
> OMP_CLAUSE_PROC_BIND_KIND (expr)
> --- gcc/tree-streamer-out.c
> +++ gcc/tree-streamer-out.c
> @@ -387,7 +387,7 @@ pack_ts_omp_clause_value_fields (struct output_block *ob,
> OMP_CLAUSE_DEPEND_KIND (expr));
> break;
> case OMP_CLAUSE_MAP:
> - bp_pack_enum (bp, omp_clause_map_kind, OMP_CLAUSE_MAP_LAST,
> + bp_pack_enum (bp, gomp_map_kind, GOMP_MAP_LAST,
> OMP_CLAUSE_MAP_KIND (expr));
> break;
> case OMP_CLAUSE_PROC_BIND:
Grüße,
Thomas
pgpU2SXskeYkm.pgp
Description: PGP signature
