| Message ID | 5571BF13.2070103@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 5, 2015 at 5:24 PM, Andrew MacLeod <amacleod@redhat.com> wrote: > There is a horrible morass of include dependencies between hash-map.h, > mem-stats.h and hash-table.h. There are even includes in both directions > (mem-stats.h and hash-map.h include each other, as do hash-map.h and > hash-table.h.. blech). Some of those files need parts of the other file to > compile, and those whole mess is quite awful. They also manage to include > vec.h into their little party 3 times as well, and it also has some icky > #ifdefs. > > So I spent some time sorting out the situation, and reduced it down to a > straight dependency list, rooted by hash-table.h. There are no double > direction includes, and no header is included more than once. Once sorted > out, I moved the root of this tree into coretypes.h since pretty much every > file requires everything in the dependency chain. This chain consists of > statistics.h, ggc.h, vec.h, hashtab.h, inchash.h, mem-stats-traits.h, > hash-map-traits.h, mem-stats.h, hash-map.h and hash-table.h. > > With hash-table.h at the root of the dependency list, I wondered how many > files actually need just that. So I flattened a source tree such that > coretypes.h included the other required include files, but each .c file > included hash-table.h. Then I tried removing the includes. It turned out > that virtually every file needs hash-table.h. Part of that is due to how > tightly integrated with mem-stats.h it is (they still need each other), and > that is used throughout the compiler. So I think it makes sense to put that > in coretypes.h. > > I also noticed that hash-set.h is included in a lot of places as well. > Wondering how much it was actually needed, I preformed the same flattening > exercise and found that only about 10% of the files in gcc core didn't need > it to compile... the rest all needed it due to hash_set<sometype> being in a > prototype parameter list or in a structure declared in a commonly used > header file (function.h, gimple-walk.h, tree-core.h, tree.h,...) . It would > be a lot of work to remove this dependency (if its even possible), so I > added hash-set.h to coretypes.h as well. rtl.h needed hash-table.h added > to the GENERATOR list, but not hash-set.. I guess the generators don't use > it much :-) > > The only other thing of note is the change to vec.h. It had an ugly set of > checks to see whether it was being used in a generator file, and if not > whether GC was available, then included it if it wasn't or provided 3 > prototypes if it wasn't suppose to be included. These allows it to compile > when GC isn't available (those routines referencing the GC functions would > never be referred to when GC isnt available). With my other changes, most > of those checks weren't necessary. I also figured it was best to simply > include those 3 prototypes for ggc_free, ggc_round_alloc_size, and > ggc_realloc all the time. When there isn't ggc.h, things remain as they are > now. If there is a ggc.h included, it will provide static confirmation that > those prototypes are up-to-date and in sync with how ggc.h defines them. > > The first patch contains all of those changes. The second one is fully > automated and removes all these headers from every other .c and .h file in > the compiler. This also included changes to many of the gen*.c routines. I > adjusted the #include list for all the *generated* .c files to also be up to > date with this patchset as well at the previous one which moved wide-int and > friends into coretypes.h > > This bootstraps with all languages enabled on x86_64-unknown-linux-gnu with > no new regressions. It also causes no failures for all the targets in > config-list.mk. > > OK for trunk? Ok. Thanks, Richard. > Andrew
On 06/05/2015 05:24 PM, Andrew MacLeod wrote: > There is a horrible morass of include dependencies between hash-map.h, mem-stats.h and hash-table.h. There are even includes in both directions (mem-stats.h and hash-map.h include each other, as do hash-map.h and hash-table.h.. blech). Some of those files need parts of the other file to compile, and those whole mess is quite awful. They also manage to include vec.h into their little party 3 times as well, and it also has some icky #ifdefs. > > So I spent some time sorting out the situation, and reduced it down to a straight dependency list, rooted by hash-table.h. There are no double direction includes, and no header is included more than once. Once sorted out, I moved the root of this tree into coretypes.h since pretty much every file requires everything in the dependency chain. This chain consists of statistics.h, ggc.h, vec.h, hashtab.h, inchash.h, mem-stats-traits.h, hash-map-traits.h, mem-stats.h, hash-map.h and hash-table.h. Hello Andrew. Thank you for solving issue related to the aforementioned cycle. Few weeks ago, I implemented memory statistics support for hash-{set|table|map}, which internally uses a hash-map and that's the reason why it was a bit awful. Anyway, nice work! Martin > > With hash-table.h at the root of the dependency list, I wondered how many files actually need just that. So I flattened a source tree such that coretypes.h included the other required include files, but each .c file included hash-table.h. Then I tried removing the includes. It turned out that virtually every file needs hash-table.h. Part of that is due to how tightly integrated with mem-stats.h it is (they still need each other), and that is used throughout the compiler. So I think it makes sense to put that in coretypes.h. > > I also noticed that hash-set.h is included in a lot of places as well. Wondering how much it was actually needed, I preformed the same flattening exercise and found that only about 10% of the files in gcc core didn't need it to compile... the rest all needed it due to hash_set<sometype> being in a prototype parameter list or in a structure declared in a commonly used header file (function.h, gimple-walk.h, tree-core.h, tree.h,...) . It would be a lot of work to remove this dependency (if its even possible), so I added hash-set.h to coretypes.h as well. rtl.h needed hash-table.h added to the GENERATOR list, but not hash-set.. I guess the generators don't use it much :-) > > The only other thing of note is the change to vec.h. It had an ugly set of checks to see whether it was being used in a generator file, and if not whether GC was available, then included it if it wasn't or provided 3 prototypes if it wasn't suppose to be included. These allows it to compile when GC isn't available (those routines referencing the GC functions would never be referred to when GC isnt available). With my other changes, most of those checks weren't necessary. I also figured it was best to simply include those 3 prototypes for ggc_free, ggc_round_alloc_size, and ggc_realloc all the time. When there isn't ggc.h, things remain as they are now. If there is a ggc.h included, it will provide static confirmation that those prototypes are up-to-date and in sync with how ggc.h defines them. > > The first patch contains all of those changes. The second one is fully automated and removes all these headers from every other .c and .h file in the compiler. This also included changes to many of the gen*.c routines. I adjusted the #include list for all the *generated* .c files to also be up to date with this patchset as well at the previous one which moved wide-int and friends into coretypes.h > > This bootstraps with all languages enabled on x86_64-unknown-linux-gnu with no new regressions. It also causes no failures for all the targets in config-list.mk. > > OK for trunk? > > Andrew
* coretypes.h: Include hash-table.h and hash-set.h for host files. * ggc.h: Don't include statistics.h> * hash-map.h: Remove all includes. * hash-set.h: Likewise. * hash-table.h: Add statistics.h, inchash.h and hash-map-traits.h to the include list. Remove <new>. * inchash.h: Remove all includes. * mem-stats.h: Likewise. * vec.h: No special processing for generators or ggc. Index: coretypes.h =================================================================== --- coretypes.h (revision 224136) +++ coretypes.h (working copy) @@ -307,6 +307,8 @@ #include "double-int.h" #include "real.h" #include "fixed-value.h" -#endif +#include "hash-table.h" +#include "hash-set.h" +#endif /* GENERATOR_FILE && !USED_FOR_TARGET */ #endif /* coretypes.h */ Index: ggc.h =================================================================== --- ggc.h (revision 224135) +++ ggc.h (working copy) @@ -20,7 +20,6 @@ #ifndef GCC_GGC_H #define GCC_GGC_H -#include "statistics.h" /* Symbols are marked with `ggc' for `gcc gc' so as not to interfere with an external gc library that might be linked in. */ Index: hash-map.h =================================================================== --- hash-map.h (revision 224135) +++ hash-map.h (working copy) @@ -21,13 +21,6 @@ #ifndef hash_map_h #define hash_map_h -#include <new> -#include <utility> -#include "hash-table.h" -#include "hash-map-traits.h" -#include "mem-stats.h" -#include "vec.h" - template<typename Key, typename Value, typename Traits> class GTY((user)) hash_map Index: hash-set.h =================================================================== --- hash-set.h (revision 224135) +++ hash-set.h (working copy) @@ -21,8 +21,6 @@ #ifndef hash_set_h #define hash_set_h -#include "hash-table.h" - /* implement default behavior for traits when types allow it. */ struct default_hashset_traits Index: hash-table.h =================================================================== --- hash-table.h (revision 224135) +++ hash-table.h (working copy) @@ -196,10 +196,13 @@ #ifndef TYPED_HASHTAB_H #define TYPED_HASHTAB_H +#include "statistics.h" #include "ggc.h" +#include "vec.h" #include "hashtab.h" -#include <new> +#include "inchash.h" #include "mem-stats-traits.h" +#include "hash-map-traits.h" template<typename, typename, typename> class hash_map; template<typename, typename> class hash_set; @@ -774,7 +777,6 @@ #include "mem-stats.h" #include "hash-map.h" -#include "vec.h" extern mem_alloc_description<mem_usage> hash_table_usage; Index: inchash.h =================================================================== --- inchash.h (revision 224136) +++ inchash.h (working copy) @@ -20,7 +20,6 @@ #ifndef INCHASH_H #define INCHASH_H 1 -#include "hashtab.h" /* This file implements an incremential hash function ADT, to be used by code that incrementially hashes a lot of unrelated data Index: mem-stats.h =================================================================== --- mem-stats.h (revision 224135) +++ mem-stats.h (working copy) @@ -1,11 +1,6 @@ #ifndef GCC_MEM_STATS_H #define GCC_MEM_STATS_H -#include "hash-map-traits.h" -#include "inchash.h" -#include "mem-stats-traits.h" -#include "vec.h" - /* Forward declaration. */ template<typename Key, typename Value, typename Traits = default_hashmap_traits> @@ -358,7 +353,6 @@ reverse_mem_map_t *m_reverse_map; }; -#include "hash-map.h" /* Returns true if instance PTR is registered by the memory description. */ Index: vec.h =================================================================== --- vec.h (revision 224135) +++ vec.h (working copy) @@ -22,39 +22,15 @@ #ifndef GCC_VEC_H #define GCC_VEC_H -/* FIXME - When compiling some of the gen* binaries, we cannot enable GC - support because the headers generated by gengtype are still not - present. In particular, the header file gtype-desc.h is missing, - so compilation may fail if we try to include ggc.h. +/* Some gen* file have no ggc support as the header file gtype-desc.h is + missing. Provide these definitions in case ggc.h has not been included. + This is not a problem because any code that runs before gengtype is built + will never need to use GC vectors.*/ - Since we use some of those declarations, we need to provide them - (even if the GC-based templates are not used). This is not a - problem because the code that runs before gengtype is built will - never need to use GC vectors. But it does force us to declare - these functions more than once. */ -#ifdef GENERATOR_FILE -#define VEC_GC_ENABLED 0 -#else -#define VEC_GC_ENABLED 1 -#endif // GENERATOR_FILE +extern void ggc_free (void *); +extern size_t ggc_round_alloc_size (size_t requested_size); +extern void *ggc_realloc (void *, size_t MEM_STAT_DECL); -#include "statistics.h" // For CXX_MEM_STAT_INFO. - -#if VEC_GC_ENABLED -#include "ggc.h" -#else -# ifndef GCC_GGC_H - /* Even if we think that GC is not enabled, the test that sets it is - weak. There are files compiled with -DGENERATOR_FILE that already - include ggc.h. We only need to provide these definitions if ggc.h - has not been included. Sigh. */ - - extern void ggc_free (void *); - extern size_t ggc_round_alloc_size (size_t requested_size); - extern void *ggc_realloc (void *, size_t MEM_STAT_DECL); -# endif // GCC_GGC_H -#endif // VEC_GC_ENABLED - /* Templated vector type and associated interfaces. The interface functions are typesafe and use inline functions,