[gomp4] Replace enum omp_clause_map_kind with enum gomp_map_kind (was: Including a file from include/ in gcc/*.h)

Hi!

On Mon, 22 Dec 2014 16:13:01 +0100, I wrote:
> 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:

I have now committed the patch to gomp-4_0-branch in the following form.
The issues raised above remain to be resolved.

In spirit against the tree.h header flattening, I had to keep the
#include "include/gomp-constants.h" in gcc/tree-core.h, because otherwise
I'd have to add it to a ton of *.c files, just for the enum gomp_map_kind
definition.

I found that in the C++ dialect used by GCC, it is not possible to
declare an enum without giving the list of enumerators.  N2764 (from
2008) resolved this by adding appropriate syntax for declaring enums,
however: "warning: scoped enums only available with -std=c++11 or
-std=gnu++11".  If it were possible to use this, we could add to
gcc/tree-core.h:

    enum gomp_map_kind : char;

... (or similar), and this way decouple the declaration (gcc/tree-core.h)
From the actual "population of it" (include/gomp-constants.h).
Alternatively, in gcc/tree-core.h:struct tree_omp_clause, we could switch
the map_kind member from enum gomp_map_kind to a char -- but that would
defeat the usage of an enum (easy pretty-printing of its enumerators in
GDB, and so on.).

Committed to gomp-4_0-branch in r219476:

commit 3421740e4c0b9262810d1c0b98f2b6c58623f868
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Mon Jan 12 16:17:35 2015 +0000

    Replace enum omp_clause_map_kind with enum gomp_map_kind.

    	include/
    	* gomp-constants.h (GOMP_MAP_LAST): Rename from
    	GOMP_MAP_VALUE_LIMIT.  Change all users.
    	(GOMP_MAP_ALLOC, GOMP_MAP_TO, GOMP_MAP_FROM, GOMP_MAP_TOFROM)
    	(GOMP_MAP_POINTER, GOMP_MAP_TO_PSET, GOMP_MAP_FORCE_PRESENT)
    	(GOMP_MAP_FORCE_DEALLOC, GOMP_MAP_FORCE_DEVICEPTR)
    	(GOMP_MAP_FORCE_ALLOC, GOMP_MAP_FORCE_TO, GOMP_MAP_FORCE_FROM)
    	(GOMP_MAP_FORCE_TOFROM): Move into a new enum gomp_map_kind.
    	gcc/
    	* tree-core.h (OMP_CLAUSE_MAP_ALLOC, OMP_CLAUSE_MAP_TO)
    	(OMP_CLAUSE_MAP_FROM, OMP_CLAUSE_MAP_TOFROM)
    	(OMP_CLAUSE_MAP_POINTER, OMP_CLAUSE_MAP_TO_PSET)
    	(OMP_CLAUSE_MAP_FORCE_ALLOC, OMP_CLAUSE_MAP_FORCE_TO)
    	(OMP_CLAUSE_MAP_FORCE_FROM, OMP_CLAUSE_MAP_FORCE_TOFROM)
    	(OMP_CLAUSE_MAP_FORCE_PRESENT, OMP_CLAUSE_MAP_FORCE_DEALLOC)
    	(OMP_CLAUSE_MAP_FORCE_DEVICEPTR, OMP_CLAUSE_MAP_LAST): Remove from
    	enum omp_clause_map_kind.  Change all users to
    	include/gomp-constants.h's enum gomp_map_kind.
    	(enum omp_clause_map_kind): Remove.

    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@219476 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog.gomp         | 13 ++++++++++
 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            | 42 +-------------------------------
 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/ChangeLog.gomp     | 10 ++++++++
 include/gomp-constants.h   | 50 +++++++++++++++++++++++++++-----------
 16 files changed, 194 insertions(+), 187 deletions(-)

Grüße,
 Thomas

[gomp4] Replace enum omp_clause_map_kind with enum gomp_map_kind (was: Including a file from include/ in gcc/*.h)

Commit Message

Comments

Patch