Message ID | 877fr42g8r.fsf@e105548-lin.cambridge.arm.com |
---|---|
State | New |
Headers | show |
On 06/16/2015 02:45 AM, Richard Sandiford wrote: > As described in the covering note, this patch replaces handle_cache_entry > with a new function keep_cache_entry. It also ensures that elements are > deleted using the proper function, so that m_n_deleted is updated. > > I couldn't tell whether the unusual name of the function > ("gt_cleare_cache") is deliberate or not, but I left it be. Short-hand for clear_entry or something similar? > > gcc/ada/ > * gcc-interface/decl.c (value_annotation_hasher::handle_cache_entry): > Delete. > (value_annotation_hasher::keep_cache_entry): New function. > * gcc-interface/utils.c (pad_type_hasher::handle_cache_entry): > Delete. > (pad_type_hasher::keep_cache_entry): New function. > > gcc/ > * hash-table.h (hash_table): Add gt_cleare_cache as a friend. > (gt_cleare_cache): Check here for deleted and empty entries. > Replace handle_cache_entry with a call to keep_cache_entry. > * hash-traits.h (ggc_cache_hasher::handle_cache_entry): Delete. > (ggc_cache_hasher::keep_cache_entry): New function. > * trans-mem.c (tm_wrapper_hasher::handle_cache_entry): Delete. > (tm_wrapper_hasher::keep_cache_entry): New function. > * tree.h (tree_decl_map_cache_hasher::handle_cache_entry): Delete. > (tree_vec_map_cache_hasher::keep_cache_entry): New function. > * tree.c (type_cache_hasher::handle_cache_entry): Delete. > (type_cache_hasher::keep_cache_entry): New function. > (tree_vec_map_cache_hasher::handle_cache_entry): Delete. > (tree_vec_map_cache_hasher::keep_cache_entry): New function. > * ubsan.c (tree_type_map_cache_hasher::handle_cache_entry): Delete. > (tree_type_map_cache_hasher::keep_cache_entry): New function. > * varasm.c (tm_clone_hasher::handle_cache_entry): Delete. > (tm_clone_hasher::keep_cache_entry): New function. > * config/i386/i386.c (dllimport_hasher::handle_cache_entry): Delete. > (dllimport_hasher::keep_cache_entry): New function. So for all the keep_cache_entry functions, I guess they're trivial enough that a function comment probably isn't needed. Presumably no good way to share the trivial implementation? OK for the trunk. jeff
Jeff Law <law@redhat.com> writes: > On 06/16/2015 02:45 AM, Richard Sandiford wrote: >> As described in the covering note, this patch replaces handle_cache_entry >> with a new function keep_cache_entry. It also ensures that elements are >> deleted using the proper function, so that m_n_deleted is updated. >> >> I couldn't tell whether the unusual name of the function >> ("gt_cleare_cache") is deliberate or not, but I left it be. > Short-hand for clear_entry or something similar? Yeah, could be. >> gcc/ada/ >> * gcc-interface/decl.c (value_annotation_hasher::handle_cache_entry): >> Delete. >> (value_annotation_hasher::keep_cache_entry): New function. >> * gcc-interface/utils.c (pad_type_hasher::handle_cache_entry): >> Delete. >> (pad_type_hasher::keep_cache_entry): New function. >> >> gcc/ >> * hash-table.h (hash_table): Add gt_cleare_cache as a friend. >> (gt_cleare_cache): Check here for deleted and empty entries. >> Replace handle_cache_entry with a call to keep_cache_entry. >> * hash-traits.h (ggc_cache_hasher::handle_cache_entry): Delete. >> (ggc_cache_hasher::keep_cache_entry): New function. >> * trans-mem.c (tm_wrapper_hasher::handle_cache_entry): Delete. >> (tm_wrapper_hasher::keep_cache_entry): New function. >> * tree.h (tree_decl_map_cache_hasher::handle_cache_entry): Delete. >> (tree_vec_map_cache_hasher::keep_cache_entry): New function. >> * tree.c (type_cache_hasher::handle_cache_entry): Delete. >> (type_cache_hasher::keep_cache_entry): New function. >> (tree_vec_map_cache_hasher::handle_cache_entry): Delete. >> (tree_vec_map_cache_hasher::keep_cache_entry): New function. >> * ubsan.c (tree_type_map_cache_hasher::handle_cache_entry): Delete. >> (tree_type_map_cache_hasher::keep_cache_entry): New function. >> * varasm.c (tm_clone_hasher::handle_cache_entry): Delete. >> (tm_clone_hasher::keep_cache_entry): New function. >> * config/i386/i386.c (dllimport_hasher::handle_cache_entry): Delete. >> (dllimport_hasher::keep_cache_entry): New function. > So for all the keep_cache_entry functions, I guess they're trivial > enough that a function comment probably isn't needed. Yeah. For cases like this where the function is implementing a defined interface (described in hash-table.h), I think it's better to only have comments for implementations that are doing something non-obvious. > Presumably no good way to share the trivial implementation? Probably not without sharing the other parts of the traits in some way. That might be another possible cleanup :-) Thanks, Richard
On 06/24/2015 02:16 AM, Richard Sandiford wrote: >> So for all the keep_cache_entry functions, I guess they're trivial >> enough that a function comment probably isn't needed. > > Yeah. For cases like this where the function is implementing a defined > interface (described in hash-table.h), I think it's better to only have > comments for implementations that are doing something non-obvious. That works for me. > >> Presumably no good way to share the trivial implementation? > > Probably not without sharing the other parts of the traits in some way. > That might be another possible cleanup :-) I'll let you decide whether or not to pursue. I'd like to hope that ICF would help us here. jeff
On Tue, Jun 16, 2015 at 09:45:56AM +0100, Richard Sandiford wrote: > As described in the covering note, this patch replaces handle_cache_entry > with a new function keep_cache_entry. It also ensures that elements are > deleted using the proper function, so that m_n_deleted is updated. Thanks for taking this off my to do list ;-) > I couldn't tell whether the unusual name of the function > ("gt_cleare_cache") is deliberate or not, but I left it be. I'm not sure what's particularly unusual about it? > + static int > + keep_cache_entry (tree_int_map *&m) I think we could now change the interface to take const T *? I imagine inlining may get rid of the extra indirection anyway, but it feels cleaner anyway. > + - An optional static function named 'keep_cache_entry'. This > + function is provided only for garbage-collected elements that > + are not marked by the normal gc mark pass. It describes what > + what should happen to the element at the end of the gc mark phase. > + The return value should be: > + - 0 if the element should be deleted > + - 1 if the element should be kept and needs to be marked > + - -1 if the element should be kept and is already marked. > + Returning -1 rather than 1 is purely an optimization. In theory using an enum seems better, but I'm not sure if the extra verbosity makes it better in practice. > + static int > + keep_cache_entry (T &e) > { > - if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && !ggc_marked_p (e)) > - e = static_cast<T> (HTAB_DELETED_ENTRY); > + return ggc_marked_p (e) ? -1 : 0; hmm, this is the only place where -1 is used right? I believe this case only works if the hash table is storing pointers to things in gc memory, and only keeps things that get marked by other paths. I believe that means -1 is only a very small optimization because in the case we return -1 all we save is the check that that pointer itself is marked. So i'm tempted to change this interface to just return a bool. Of course it would be nice if the compiler could inline enough to actually optimize out the redundant check if the pointer is makred, but that might take some shuffling code around. thanks for cleaning this up, and sorry my response is delayed. Trev
Trevor Saunders <tbsaunde@tbsaunde.org> writes: > On Tue, Jun 16, 2015 at 09:45:56AM +0100, Richard Sandiford wrote: >> I couldn't tell whether the unusual name of the function >> ("gt_cleare_cache") is deliberate or not, but I left it be. > > I'm not sure what's particularly unusual about it? "cleare" rather than "clear". >> + static int >> + keep_cache_entry (tree_int_map *&m) > > I think we could now change the interface to take const T *? I imagine > inlining may get rid of the extra indirection anyway, but it feels > cleaner anyway. Yeah, good point. >> + - An optional static function named 'keep_cache_entry'. This >> + function is provided only for garbage-collected elements that >> + are not marked by the normal gc mark pass. It describes what >> + what should happen to the element at the end of the gc mark phase. >> + The return value should be: >> + - 0 if the element should be deleted >> + - 1 if the element should be kept and needs to be marked >> + - -1 if the element should be kept and is already marked. >> + Returning -1 rather than 1 is purely an optimization. > > In theory using an enum seems better, but I'm not sure if the extra > verbosity makes it better in practice. Yeah, I wondered about an enum but it seemed like overkill. >> + static int >> + keep_cache_entry (T &e) >> { >> - if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && >> !ggc_marked_p (e)) >> - e = static_cast<T> (HTAB_DELETED_ENTRY); >> + return ggc_marked_p (e) ? -1 : 0; > > hmm, this is the only place where -1 is used right? I believe this > case only works if the hash table is storing pointers to things in gc > memory, and only keeps things that get marked by other paths. I > believe that means -1 is only a very small optimization because in the > case we return -1 all we save is the check that that pointer itself is > marked. Right. But it's an optimisation that we had before and I think we should keep it. The -1 case is in the generic traits rather than "user" code. > So i'm tempted to change this interface to just return a bool. > Of course it would be nice if the compiler could inline enough to > actually optimize out the redundant check if the pointer is makred, but > that might take some shuffling code around. In practice all calls will return from {0, 1} or {0, -1}, so assuming sensible inlining, no caller will use all three paths. Thanks, Richard
On Wed, Jul 01, 2015 at 10:59:15AM +0100, Richard Sandiford wrote: > Trevor Saunders <tbsaunde@tbsaunde.org> writes: > > On Tue, Jun 16, 2015 at 09:45:56AM +0100, Richard Sandiford wrote: > >> I couldn't tell whether the unusual name of the function > >> ("gt_cleare_cache") is deliberate or not, but I left it be. > > > > I'm not sure what's particularly unusual about it? > > "cleare" rather than "clear". oh! I didn't notice that but yeah its wierd probably just being a bad typist. > >> + static int > >> + keep_cache_entry (tree_int_map *&m) > > > > I think we could now change the interface to take const T *? I imagine > > inlining may get rid of the extra indirection anyway, but it feels > > cleaner anyway. > > Yeah, good point. > > >> + - An optional static function named 'keep_cache_entry'. This > >> + function is provided only for garbage-collected elements that > >> + are not marked by the normal gc mark pass. It describes what > >> + what should happen to the element at the end of the gc mark phase. > >> + The return value should be: > >> + - 0 if the element should be deleted > >> + - 1 if the element should be kept and needs to be marked > >> + - -1 if the element should be kept and is already marked. > >> + Returning -1 rather than 1 is purely an optimization. > > > > In theory using an enum seems better, but I'm not sure if the extra > > verbosity makes it better in practice. > > Yeah, I wondered about an enum but it seemed like overkill. > > >> + static int > >> + keep_cache_entry (T &e) > >> { > >> - if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && > >> !ggc_marked_p (e)) > >> - e = static_cast<T> (HTAB_DELETED_ENTRY); > >> + return ggc_marked_p (e) ? -1 : 0; > > > > hmm, this is the only place where -1 is used right? I believe this > > case only works if the hash table is storing pointers to things in gc > > memory, and only keeps things that get marked by other paths. I > > believe that means -1 is only a very small optimization because in the > > case we return -1 all we save is the check that that pointer itself is > > marked. > > Right. But it's an optimisation that we had before and I think we > should keep it. The -1 case is in the generic traits rather than > "user" code. > > > So i'm tempted to change this interface to just return a bool. > > Of course it would be nice if the compiler could inline enough to > > actually optimize out the redundant check if the pointer is makred, but > > that might take some shuffling code around. > > In practice all calls will return from {0, 1} or {0, -1}, so assuming > sensible inlining, no caller will use all three paths. yeah Trev > > Thanks, > Richard >
Index: gcc/ada/gcc-interface/decl.c =================================================================== --- gcc/ada/gcc-interface/decl.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/ada/gcc-interface/decl.c 2015-06-15 16:04:47.363633798 +0100 @@ -149,16 +149,10 @@ struct value_annotation_hasher : ggc_cac return a->base.from == b->base.from; } - static void - handle_cache_entry (tree_int_map *&m) + static int + keep_cache_entry (tree_int_map *&m) { - extern void gt_ggc_mx (tree_int_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->base.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_int_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (m->base.from); } }; Index: gcc/ada/gcc-interface/utils.c =================================================================== --- gcc/ada/gcc-interface/utils.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/ada/gcc-interface/utils.c 2015-06-15 16:04:47.359633844 +0100 @@ -243,7 +243,7 @@ struct pad_type_hasher : ggc_cache_hashe { static inline hashval_t hash (pad_type_hash *t) { return t->hash; } static bool equal (pad_type_hash *a, pad_type_hash *b); - static void handle_cache_entry (pad_type_hash *&); + static int keep_cache_entry (pad_type_hash *&); }; static GTY ((cache)) @@ -1170,16 +1170,10 @@ make_type_from_size (tree type, tree siz /* See if the data pointed to by the hash table slot is marked. */ -void -pad_type_hasher::handle_cache_entry (pad_type_hash *&t) +int +pad_type_hasher::keep_cache_entry (pad_type_hash *&t) { - extern void gt_ggc_mx (pad_type_hash *&); - if (t == HTAB_EMPTY_ENTRY || t == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (t->type)) - gt_ggc_mx (t); - else - t = static_cast<pad_type_hash *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (t->type); } /* Return true iff the padded types are equivalent. */ Index: gcc/hash-table.h =================================================================== --- gcc/hash-table.h 2015-06-15 16:04:47.379633614 +0100 +++ gcc/hash-table.h 2015-06-15 16:04:47.355633890 +0100 @@ -52,6 +52,16 @@ Software Foundation; either version 3, o individual elements of the table need to be disposed of (e.g., when deleting a hash table, removing elements from the table, etc). + - An optional static function named 'keep_cache_entry'. This + function is provided only for garbage-collected elements that + are not marked by the normal gc mark pass. It describes what + what should happen to the element at the end of the gc mark phase. + The return value should be: + - 0 if the element should be deleted + - 1 if the element should be kept and needs to be marked + - -1 if the element should be kept and is already marked. + Returning -1 rather than 1 is purely an optimization. + 3. The type of the hash table itself. (More later.) In very special circumstances, users may need to know about a fourth type. @@ -584,6 +594,8 @@ struct mark_empty_helper<Type *, Traits, template<typename T> friend void gt_pch_nx (hash_table<T> *, gt_pointer_operator, void *); + template<typename T> friend void gt_cleare_cache (hash_table<T> *); + value_type *alloc_entries (size_t n CXX_MEM_STAT_INFO) const; value_type *find_empty_slot_for_expand (hashval_t); void expand (); @@ -1131,12 +1143,20 @@ gt_pch_nx (hash_table<D> *h, gt_pointer_ inline void gt_cleare_cache (hash_table<H> *h) { + extern void gt_ggc_mx (typename H::value_type &t); + typedef hash_table<H> table; if (!h) return; - for (typename hash_table<H>::iterator iter = h->begin (); iter != h->end (); - ++iter) - H::handle_cache_entry (*iter); + for (typename table::iterator iter = h->begin (); iter != h->end (); ++iter) + if (!table::is_empty (*iter) && !table::is_deleted (*iter)) + { + int res = H::keep_cache_entry (*iter); + if (res == 0) + h->clear_slot (&*iter); + else if (res != -1) + gt_ggc_mx (*iter); + } } #endif /* TYPED_HASHTAB_H */ Index: gcc/hash-traits.h =================================================================== --- gcc/hash-traits.h 2015-06-15 16:04:47.379633614 +0100 +++ gcc/hash-traits.h 2015-06-15 16:04:47.355633890 +0100 @@ -146,13 +146,10 @@ struct ggc_cache_hasher op (&p, cookie); } - /* Clear out entries if they are about to be gc'd. */ - - static void - handle_cache_entry (T &e) + static int + keep_cache_entry (T &e) { - if (e != HTAB_EMPTY_ENTRY && e != HTAB_DELETED_ENTRY && !ggc_marked_p (e)) - e = static_cast<T> (HTAB_DELETED_ENTRY); + return ggc_marked_p (e) ? -1 : 0; } }; Index: gcc/trans-mem.c =================================================================== --- gcc/trans-mem.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/trans-mem.c 2015-06-15 16:04:47.359633844 +0100 @@ -483,17 +483,11 @@ struct tm_wrapper_hasher : ggc_cache_has return a->base.from == b->base.from; } - static void - handle_cache_entry (tree_map *&m) - { - extern void gt_ggc_mx (tree_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->base.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_map *> (HTAB_DELETED_ENTRY); - } + static int + keep_cache_entry (tree_map *&m) + { + return ggc_marked_p (m->base.from); + } }; static GTY((cache)) hash_table<tm_wrapper_hasher> *tm_wrap_map; Index: gcc/tree.h =================================================================== --- gcc/tree.h 2015-06-15 16:04:47.379633614 +0100 +++ gcc/tree.h 2015-06-15 16:04:47.375633660 +0100 @@ -4637,16 +4637,10 @@ struct tree_decl_map_cache_hasher : ggc_ return tree_decl_map_eq (a, b); } - static void - handle_cache_entry (tree_decl_map *&m) + static int + keep_cache_entry (tree_decl_map *&m) { - extern void gt_ggc_mx (tree_decl_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->base.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_decl_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (m->base.from); } }; Index: gcc/tree.c =================================================================== --- gcc/tree.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/tree.c 2015-06-15 16:04:47.375633660 +0100 @@ -203,16 +203,10 @@ struct type_cache_hasher : ggc_cache_has static hashval_t hash (type_hash *t) { return t->hash; } static bool equal (type_hash *a, type_hash *b); - static void - handle_cache_entry (type_hash *&t) + static int + keep_cache_entry (type_hash *&t) { - extern void gt_ggc_mx (type_hash *&); - if (t == HTAB_DELETED_ENTRY || t == HTAB_EMPTY_ENTRY) - return; - else if (ggc_marked_p (t->type)) - gt_ggc_mx (t); - else - t = static_cast<type_hash *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (t->type); } }; @@ -261,7 +255,7 @@ static GTY ((cache)) static GTY ((cache)) hash_table<tree_decl_map_cache_hasher> *value_expr_for_decl; - struct tree_vec_map_cache_hasher : ggc_cache_hasher<tree_vec_map *> +struct tree_vec_map_cache_hasher : ggc_cache_hasher<tree_vec_map *> { static hashval_t hash (tree_vec_map *m) { return DECL_UID (m->base.from); } @@ -271,16 +265,10 @@ static GTY ((cache)) return a->base.from == b->base.from; } - static void - handle_cache_entry (tree_vec_map *&m) + static int + keep_cache_entry (tree_vec_map *&m) { - extern void gt_ggc_mx (tree_vec_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->base.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_vec_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (m->base.from); } }; Index: gcc/ubsan.c =================================================================== --- gcc/ubsan.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/ubsan.c 2015-06-15 16:04:47.359633844 +0100 @@ -99,16 +99,10 @@ struct tree_type_map_cache_hasher : ggc_ return a->type.from == b->type.from; } - static void - handle_cache_entry (tree_type_map *&m) + static int + keep_cache_entry (tree_type_map *&m) { - extern void gt_ggc_mx (tree_type_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->type.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_type_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (m->type.from); } }; Index: gcc/varasm.c =================================================================== --- gcc/varasm.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/varasm.c 2015-06-15 16:04:47.375633660 +0100 @@ -5797,16 +5797,10 @@ struct tm_clone_hasher : ggc_cache_hashe static hashval_t hash (tree_map *m) { return tree_map_hash (m); } static bool equal (tree_map *a, tree_map *b) { return tree_map_eq (a, b); } - static void - handle_cache_entry (tree_map *&e) + static int + keep_cache_entry (tree_map *&e) { - extern void gt_ggc_mx (tree_map *&); - if (e == HTAB_EMPTY_ENTRY || e == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (e->base.from)) - gt_ggc_mx (e); - else - e = static_cast<tree_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (e->base.from); } }; Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2015-06-15 16:04:47.379633614 +0100 +++ gcc/config/i386/i386.c 2015-06-15 16:04:47.371633705 +0100 @@ -14212,16 +14212,10 @@ struct dllimport_hasher : ggc_cache_hash return a->base.from == b->base.from; } - static void - handle_cache_entry (tree_map *&m) + static int + keep_cache_entry (tree_map *&m) { - extern void gt_ggc_mx (tree_map *&); - if (m == HTAB_EMPTY_ENTRY || m == HTAB_DELETED_ENTRY) - return; - else if (ggc_marked_p (m->base.from)) - gt_ggc_mx (m); - else - m = static_cast<tree_map *> (HTAB_DELETED_ENTRY); + return ggc_marked_p (m->base.from); } };