Re: Including a file from include/ in gcc/*.h (was: [gomp4] Use include/gomp-constants.h more actively)

[Thomas Schwinge <thomas at codesourcery dot com>]

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 <jakub@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 <thomas@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

Attachment: pgpUaoJ6GVUOo.pgp
Description: PGP signature