Issue 4956041: [pph] Independent pre-loaded cache for common nodes (Closed)
|
Created:
12 years, 8 months ago by Gabriel Charette Modified:
12 years, 8 months ago CC:
gcc-patches_gcc.gnu.org Visibility:
Public. |
Patch Set 1 #
Total comments: 3
MessagesTotal messages: 4
This patch creates an independent cache, pph_preloaded_cache, which contains all common nodes which used to be directly added to each stream's cache. The streams' cache now only contain nodes which really belong to them. The preloaded cache is last in the lookup on the way out (potentially allowing us to play tricks later if we need to, i.e. by adding a modified preloaded structure's reference in the main cache to get the hit we want earlier and control the result...?) Tested with bootstrap and pph regression testing. Cheers, Gab 2011-08-24 Gabriel Charette <gchare@google.com> * pph-streamer-in.c (pph_is_reference_marker): Handle PPH_RECORD_PREF. (pph_in_start_record): Likewise. * pph-streamer-out.c (pph_out_start_record): Add extra lookup in pph_preloaded_cache if lookup in current and included streams failed. * pph-streamer.c (pph_preloaded_cache): New. (pph_cache_preload): Change first parameter to a pph_pickle_cache* from a pph_stream*. Update all users. (pph_cache_init): New. (pph_init_preloaded_cache): New local variable. (pph_stream_open): Use pph_cache_init. Do not call pph_cache_preload. (pph_cache_get): Add a new pph_record_marker parameter which we use to determine to type of get to do. Update all users. * pph-streamer.h (enum pph_record_marker): Add PPH_RECORD_PREF. (pph_in_record_marker): Handle PPH_RECORD_PREF. * pph.c (pph_init): Call pph_init_preloaded_cache. diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c index 720da6a..1561f88 100644 --- a/gcc/cp/pph-streamer-in.c +++ b/gcc/cp/pph-streamer-in.c @@ -145,12 +145,14 @@ pph_init_read (pph_stream *stream) } -/* Return true if MARKER is either PPH_RECORD_IREF or PPH_RECORD_XREF. */ +/* Return true if MARKER is PPH_RECORD_IREF, PPH_RECORD_XREF, + or PPH_RECORD_PREF. */ static inline bool pph_is_reference_marker (enum pph_record_marker marker) { - return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF; + return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF + || marker == PPH_RECORD_PREF; } @@ -189,8 +191,10 @@ pph_in_start_record (pph_stream *stream, unsigned *include_ix_p, /* For PPH_RECORD_START and PPH_RECORD_IREF markers, read the streamer cache slot where we should store or find the - rematerialized data structure (see description above). */ - if (marker == PPH_RECORD_START || marker == PPH_RECORD_IREF) + rematerialized data structure (see description above). + Also read the preloaded cache slot in IX for PPH_RECORD_PREF. */ + if (marker == PPH_RECORD_START || marker == PPH_RECORD_IREF + || marker == PPH_RECORD_PREF) *cache_ix_p = pph_in_uint (stream); else if (marker == PPH_RECORD_XREF) { @@ -407,13 +411,13 @@ pph_in_cxx_binding_1 (pph_stream *stream) cxx_binding *cb; tree value, type; enum pph_record_marker marker; - unsigned ix, include_ix; + unsigned ix, image_ix; - marker = pph_in_start_record (stream, &include_ix, &ix); + marker = pph_in_start_record (stream, &image_ix, &ix); if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cxx_binding *) pph_cache_get (&stream->cache, include_ix, ix); + return (cxx_binding *) pph_cache_get (&stream->cache, image_ix, ix, marker); value = pph_in_tree (stream); type = pph_in_tree (stream); @@ -461,7 +465,8 @@ pph_in_class_binding (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_class_binding *) pph_cache_get (&stream->cache, image_ix, ix); + return (cp_class_binding *) pph_cache_get (&stream->cache, image_ix, ix, + marker); ALLOC_AND_REGISTER (&stream->cache, ix, cb, ggc_alloc_cleared_cp_class_binding ()); @@ -485,7 +490,8 @@ pph_in_label_binding (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_label_binding *) pph_cache_get (&stream->cache, image_ix, ix); + return (cp_label_binding *) pph_cache_get (&stream->cache, image_ix, ix, + marker); ALLOC_AND_REGISTER (&stream->cache, ix, lb, ggc_alloc_cleared_cp_label_binding ()); @@ -526,7 +532,8 @@ pph_in_binding_level (pph_stream *stream, cp_binding_level *to_register) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (cp_binding_level *) pph_cache_get (&stream->cache, image_ix, ix); + return (cp_binding_level *) pph_cache_get (&stream->cache, image_ix, ix, + marker); /* If TO_REGISTER is set, register that binding level instead of the newly allocated binding level into slot IX. */ @@ -604,7 +611,7 @@ pph_in_c_language_function (pph_stream *stream) return NULL; else if (pph_is_reference_marker (marker)) return (struct c_language_function *) pph_cache_get (&stream->cache, - image_ix, ix); + image_ix, ix, marker); ALLOC_AND_REGISTER (&stream->cache, ix, clf, ggc_alloc_cleared_c_language_function ()); @@ -630,7 +637,7 @@ pph_in_language_function (pph_stream *stream) return NULL; else if (pph_is_reference_marker (marker)) return (struct language_function *) pph_cache_get (&stream->cache, - image_ix, ix); + image_ix, ix, marker); ALLOC_AND_REGISTER (&stream->cache, ix, lf, ggc_alloc_cleared_language_function ()); @@ -828,7 +835,8 @@ pph_in_lang_specific (pph_stream *stream, tree decl) else if (pph_is_reference_marker (marker)) { DECL_LANG_SPECIFIC (decl) = - (struct lang_decl *) pph_cache_get (&stream->cache, image_ix, ix); + (struct lang_decl *) pph_cache_get (&stream->cache, image_ix, ix, + marker); return; } @@ -923,7 +931,7 @@ pph_in_sorted_fields_type (pph_stream *stream) return NULL; else if (pph_is_reference_marker (marker)) return (struct sorted_fields_type *) pph_cache_get (&stream->cache, - image_ix, ix); + image_ix, ix, marker); num_fields = pph_in_uint (stream); ALLOC_AND_REGISTER (&stream->cache, ix, v, @@ -1005,7 +1013,7 @@ pph_in_lang_type_class (pph_stream *stream, struct lang_type_class *ltc) } else if (pph_is_reference_marker (marker)) ltc->nested_udts = (binding_table) pph_cache_get (&stream->cache, - image_ix, ix); + image_ix, ix, marker); ltc->as_base = pph_in_tree (stream); ltc->pure_virtuals = pph_in_tree_vec (stream); @@ -1044,7 +1052,8 @@ pph_in_lang_type (pph_stream *stream) if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (struct lang_type *) pph_cache_get (&stream->cache, image_ix, ix); + return (struct lang_type *) pph_cache_get (&stream->cache, image_ix, ix, + marker); ALLOC_AND_REGISTER (&stream->cache, ix, lt, ggc_alloc_cleared_lang_type (sizeof (struct lang_type))); @@ -1934,7 +1943,7 @@ pph_read_tree (struct lto_input_block *ib ATTRIBUTE_UNUSED, if (marker == PPH_RECORD_END) return NULL; else if (pph_is_reference_marker (marker)) - return (tree) pph_cache_get (&stream->cache, image_ix, ix); + return (tree) pph_cache_get (&stream->cache, image_ix, ix, marker); /* We did not find the tree in the pickle cache, allocate the tree by reading the header fields (different tree nodes need to be diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c index 83f98c3..5571cb0 100644 --- a/gcc/cp/pph-streamer-out.c +++ b/gcc/cp/pph-streamer-out.c @@ -219,6 +219,14 @@ pph_out_start_record (pph_stream *stream, void *data) return false; } + /* DATA is not in any stream's cache. See if it is a preloaded node. */ + if (pph_cache_lookup (NULL, data, &ix)) + { + pph_out_record_marker (stream, PPH_RECORD_PREF); + pph_out_uint (stream, ix); + return false; + } + /* DATA is in none of the pickle caches, add it to STREAM's pickle cache and tell the caller that it should pickle DATA out. */ pph_cache_add (&stream->cache, data, &ix); diff --git a/gcc/cp/pph-streamer.c b/gcc/cp/pph-streamer.c index 8ac3ddc..a29dac9 100644 --- a/gcc/cp/pph-streamer.c +++ b/gcc/cp/pph-streamer.c @@ -40,35 +40,32 @@ along with GCC; see the file COPYING3. If not see them until the end of parsing (when pph_reader_finish is called). */ VEC(pph_stream_ptr, heap) *pph_read_images; -/* Pre-load common tree nodes into the pickle cache in STREAM. These - nodes are always built by the front end, so there is no need to - pickle them. +/* A cache of pre-loaded common tree nodes. */ +static pph_pickle_cache *pph_preloaded_cache; - FIXME pph - Every stream will have its own pickle cache. Many - entries in all those caches will be the same. Implement a common - cache for everything we preload here so that we do not have to - preload every cache we instantiate. */ +/* Pre-load common tree nodes into CACHE. These nodes are always built by the + front end, so there is no need to pickle them. */ static void -pph_cache_preload (pph_stream *stream) +pph_cache_preload (pph_pickle_cache *cache) { unsigned i; for (i = itk_char; i < itk_none; i++) - pph_cache_add (&stream->cache, integer_types[i], NULL); + pph_cache_add (cache, integer_types[i], NULL); for (i = 0; i < TYPE_KIND_LAST; i++) - pph_cache_add (&stream->cache, sizetype_tab[i], NULL); + pph_cache_add (cache, sizetype_tab[i], NULL); /* global_trees[] can have NULL entries in it. Skip them. */ for (i = 0; i < TI_MAX; i++) if (global_trees[i]) - pph_cache_add (&stream->cache, global_trees[i], NULL); + pph_cache_add (cache, global_trees[i], NULL); /* c_global_trees[] can have NULL entries in it. Skip them. */ for (i = 0; i < CTI_MAX; i++) if (c_global_trees[i]) - pph_cache_add (&stream->cache, c_global_trees[i], NULL); + pph_cache_add (cache, c_global_trees[i], NULL); /* cp_global_trees[] can have NULL entries in it. Skip them. */ for (i = 0; i < CPTI_MAX; i++) @@ -78,13 +75,13 @@ pph_cache_preload (pph_stream *stream) continue; if (cp_global_trees[i]) - pph_cache_add (&stream->cache, cp_global_trees[i], NULL); + pph_cache_add (cache, cp_global_trees[i], NULL); } /* Add other well-known nodes that should always be taken from the current compilation context. */ - pph_cache_add (&stream->cache, global_namespace, NULL); - pph_cache_add (&stream->cache, DECL_CONTEXT (global_namespace), NULL); + pph_cache_add (cache, global_namespace, NULL); + pph_cache_add (cache, DECL_CONTEXT (global_namespace), NULL); } @@ -101,6 +98,25 @@ pph_hooks_init (void) } +/* Initialize an empty pickle CACHE. */ + +static void +pph_cache_init (pph_pickle_cache *cache) +{ + cache->v = NULL; + cache->m = pointer_map_create (); +} + + +void +pph_init_preloaded_cache (void) +{ + pph_preloaded_cache = XCNEW (pph_pickle_cache); + pph_cache_init (pph_preloaded_cache); + pph_cache_preload (pph_preloaded_cache); +} + + /* Create a new PPH stream to be stored on the file called NAME. MODE is passed to fopen directly. */ @@ -120,15 +136,13 @@ pph_stream_open (const char *name, const char *mode) stream->file = f; stream->name = xstrdup (name); stream->write_p = (strchr (mode, 'w') != NULL); - stream->cache.m = pointer_map_create (); + pph_cache_init (&stream->cache); stream->symtab.m = pointer_set_create (); if (stream->write_p) pph_init_write (stream); else pph_init_read (stream); - pph_cache_preload (stream); - return stream; } @@ -400,7 +414,8 @@ pph_cache_insert_at (pph_pickle_cache *cache, void *data, unsigned ix) /* Return true if DATA exists in CACHE. If IX_P is not NULL, store the cache - slot where DATA resides in *IX_P (or (unsigned)-1 if DATA is not found). */ + slot where DATA resides in *IX_P (or (unsigned)-1 if DATA is not found). + If CACHE is NULL use pph_preloaded_cache by default. */ bool pph_cache_lookup (pph_pickle_cache *cache, void *data, unsigned *ix_p) @@ -409,6 +424,9 @@ pph_cache_lookup (pph_pickle_cache *cache, void *data, unsigned *ix_p) unsigned ix; bool existed_p; + if (cache == NULL) + cache = pph_preloaded_cache; + map_slot = pointer_map_contains (cache->m, data); if (map_slot == NULL) { @@ -502,24 +520,35 @@ pph_cache_add (pph_pickle_cache *cache, void *data, unsigned *ix_p) } -/* Return the pointer at slot IX in STREAM's pickle cache. If INCLUDE_IX - is not -1U, then instead of looking up in STREAM's pickle cache, - instead of looking up in CACHE, the pointer is looked up in the CACHE of - pph_read_images[INCLUDE_IX]. */ +/* Return the pointer at slot IX in STREAM_CACHE if MARKER is an IREF. IF + MARKER is a XREF then instead of looking up in STREAM_CACHE, the pointer is + looked up in the CACHE of pph_read_images[INCLUDE_IX]. Finally if MARKER is + a PREF return the pointer at slot IX in the preloaded cache. */ void * -pph_cache_get (pph_pickle_cache *cache, unsigned include_ix, unsigned ix) +pph_cache_get (pph_pickle_cache *stream_cache, unsigned include_ix, unsigned ix, + enum pph_record_marker marker) { void *data; - pph_pickle_cache *img_cache; + pph_pickle_cache *cache; - /* Determine which image's pickle cache to use. */ - if (include_ix == (unsigned) -1) - img_cache = cache; - else - img_cache = &VEC_index (pph_stream_ptr, pph_read_images, include_ix)->cache; + /* Determine which cache to use. */ + switch (marker) + { + case PPH_RECORD_IREF: + cache = stream_cache; + break; + case PPH_RECORD_XREF: + cache = &VEC_index (pph_stream_ptr, pph_read_images, include_ix)->cache; + break; + case PPH_RECORD_PREF: + cache = pph_preloaded_cache; + break; + default: + gcc_unreachable (); + } - data = VEC_index (void_p, img_cache->v, ix); + data = VEC_index (void_p, cache->v, ix); gcc_assert (data); return data; } diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h index a51db3b..6ee643e 100644 --- a/gcc/cp/pph-streamer.h +++ b/gcc/cp/pph-streamer.h @@ -47,7 +47,12 @@ enum pph_record_marker { indices: (1) the index into the include table where the other image lives, and (2) the cache slot into that image's pickle cache where the data resides. */ - PPH_RECORD_XREF + PPH_RECORD_XREF, + + /* Preloaded reference. This marker indicates that this data is a preloaded + node created by the front-end at the beginning of compilation, which we + do not need to stream out as it will already exist on the way in. */ + PPH_RECORD_PREF }; /* Symbol table markers. These are all the symbols that need to be @@ -213,6 +218,7 @@ typedef struct pph_stream { #define PPHF_NO_XREFS (1 << 1) /* In pph-streamer.c. */ +void pph_init_preloaded_cache (void); pph_stream *pph_stream_open (const char *, const char *); void pph_stream_close (pph_stream *); void pph_trace_tree (pph_stream *, tree); @@ -227,7 +233,8 @@ void pph_cache_insert_at (pph_pickle_cache *, void *, unsigned); bool pph_cache_lookup (pph_pickle_cache *, void *, unsigned *); bool pph_cache_lookup_in_includes (void *, unsigned *, unsigned *); bool pph_cache_add (pph_pickle_cache *, void *, unsigned *); -void *pph_cache_get (pph_pickle_cache *, unsigned, unsigned); +void *pph_cache_get (pph_pickle_cache *, unsigned, unsigned, + enum pph_record_marker); /* In pph-streamer-out.c. */ void pph_flush_buffers (pph_stream *); @@ -529,7 +536,8 @@ pph_in_record_marker (pph_stream *stream) gcc_assert (m == PPH_RECORD_START || m == PPH_RECORD_END || m == PPH_RECORD_IREF - || m == PPH_RECORD_XREF); + || m == PPH_RECORD_XREF + || m == PPH_RECORD_PREF); return m; } diff --git a/gcc/cp/pph.c b/gcc/cp/pph.c index 74f48fc..9b1c63c 100644 --- a/gcc/cp/pph.c +++ b/gcc/cp/pph.c @@ -168,6 +168,8 @@ pph_init (void) if (pph_out_file != NULL) pph_writer_init (); + pph_init_preloaded_cache (); + pph_read_images = NULL; } -- This patch is available for review at http://codereview.appspot.com/4956041
Sign in to reply to this message.
OK with a couple of nits below. Diego. http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c File gcc/cp/pph-streamer-in.c (right): http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode155 gcc/cp/pph-streamer-in.c:155: || marker == PPH_RECORD_PREF; 154 return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF 155 || marker == PPH_RECORD_PREF; Align these vertically. http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode197 gcc/cp/pph-streamer-in.c:197: || marker == PPH_RECORD_PREF) 154 return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF 155 || marker == PPH_RECORD_PREF; Likewise. http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer.c File gcc/cp/pph-streamer.c (right): http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer.c#newcode530 gcc/cp/pph-streamer.c:530: enum pph_record_marker marker) 529 pph_cache_get (pph_pickle_cache *stream_cache, unsigned include_ix, unsigned ix, 530 enum pph_record_marker marker) Do we really need the MARKER argument? If STREAM_CACHE is NULL, we use INCLUDE_IX to determine if they want an external reference or the preloaded cache. Though, I guess adding MARKER does not hurt and makes the code easier to read. Never mind.
Sign in to reply to this message.
On Thu, Aug 25, 2011 at 5:35 AM, <dnovillo@google.com> wrote: > OK with a couple of nits below. > > > Diego. > > > http://codereview.appspot.com/**4956041/diff/1/gcc/cp/pph-**streamer-in.c<htt... > File gcc/cp/pph-streamer-in.c (right): > > http://codereview.appspot.com/**4956041/diff/1/gcc/cp/pph-** > streamer-in.c#newcode155<http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode155> > gcc/cp/pph-streamer-in.c:155: || marker == PPH_RECORD_PREF; > 154 return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF > 155 || marker == PPH_RECORD_PREF; > > Align these vertically. > What do you mean? the || is aligned with the 'marker' entry above it. Do you want the || to be aligned? i.e. marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF || marker == PPH_RECORD_PREF ? > > http://codereview.appspot.com/**4956041/diff/1/gcc/cp/pph-** > streamer-in.c#newcode197<http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode197> > gcc/cp/pph-streamer-in.c:197: || marker == PPH_RECORD_PREF) > 154 return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF > 155 || marker == PPH_RECORD_PREF; > > Likewise. > > http://codereview.appspot.com/**4956041/diff/1/gcc/cp/pph-**streamer.c<http:/... > File gcc/cp/pph-streamer.c (right): > > http://codereview.appspot.com/**4956041/diff/1/gcc/cp/pph-** > streamer.c#newcode530<http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer.c#newcode530> > gcc/cp/pph-streamer.c:530: enum pph_record_marker marker) > 529 pph_cache_get (pph_pickle_cache *stream_cache, unsigned include_ix, > unsigned ix, > 530 enum pph_record_marker marker) > > Do we really need the MARKER argument? If STREAM_CACHE is NULL, we use > INCLUDE_IX to determine if they want an external reference or the > preloaded cache. > > Though, I guess adding MARKER does not hurt and makes the code easier to > read. Never mind. > > Ya I had the same thought and same resolution. Relying on include_ix == -1U was already sketchy imo, and now adding another trigger on stream == NULL was just too much assumptions and dependence on the rest of the implementation I thought. > http://codereview.appspot.com/**4956041/<http://codereview.appspot.com/4956041/> > Cheers, Gab
Sign in to reply to this message.
On Thu, Aug 25, 2011 at 13:17, Gabriel Charette <gchare@google.com> wrote: > What do you mean? the || is aligned with the 'marker' entry above it. > Do you want the || to be aligned? i.e. > marker == PPH_RECORD_IREF > || marker == PPH_RECORD_XREF > || marker == PPH_RECORD_PREF > ? Yeah, this way. Every operand of the predicate in its own line. > Ya I had the same thought and same resolution. Relying on include_ix == -1U > was already sketchy imo, and now adding another trigger on stream == NULL > was just too much assumptions and dependence on the rest of the > implementation I thought. Sounds good. Diego.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
