Re: [02/13] Replace handle_cache_entry with new interface

[Richard Sandiford <richard dot sandiford at arm dot com>]

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