This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [02/13] Replace handle_cache_entry with new interface
- From: Trevor Saunders <tbsaunde at tbsaunde dot org>
- To: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, richard dot sandiford at arm dot com
- Date: Wed, 1 Jul 2015 07:24:01 -0400
- Subject: Re: [02/13] Replace handle_cache_entry with new interface
- Authentication-results: sourceware.org; auth=none
- References: <87fv5s2gej dot fsf at e105548-lin dot cambridge dot arm dot com> <877fr42g8r dot fsf at e105548-lin dot cambridge dot arm dot com> <20150701094031 dot GC11111 at tsaunders-iceball dot corp dot tor1 dot mozilla dot com> <87d20cnqq4 dot fsf at e105548-lin dot cambridge dot arm dot com>
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
>