Re: [RFC][PATCH] Do refactoring of attribute functions and move them to attribs.[hc].

["Martin Liška" <mliska@suse.cz>]

On 09/12/2017 01:39 PM, Jakub Jelinek wrote:
> On Tue, Sep 12, 2017 at 01:31:47PM +0200, Martin Liška wrote:
>> >From a40c06fc06afcb7bb886d7a3106e6da631a48430 Mon Sep 17 00:00:00 2001
>> From: marxin <mliska@suse.cz>
>> Date: Tue, 12 Sep 2017 13:30:39 +0200
>> Subject: [PATCH] Reduce lookup_attribute memory footprint.
>>
>> gcc/ChangeLog:
>>
>> 2017-09-12  Martin Liska  <mliska@suse.cz>
>>
>> 	* attribs.c (private_lookup_attribute): New function.
>> 	* attribs.h (private_lookup_attribute): Declared here.
>> 	(lookup_attribute): Called from this place.
>> ---
>>  gcc/attribs.c | 17 +++++++++++++++++
>>  gcc/attribs.h | 17 ++++++-----------
>>  2 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/gcc/attribs.c b/gcc/attribs.c
>> index b8f58a74596..9064434e5f2 100644
>> --- a/gcc/attribs.c
>> +++ b/gcc/attribs.c
>> @@ -1584,3 +1584,20 @@ attribute_list_contained (const_tree l1, const_tree l2)
>>  
>>    return 1;
>>  }
>> +
>> +
> 
> Can you please add a function comment (and in addition to arguments explain
> there why lookup_attribute is split into the two parts)?

Yes.

> 
>> +tree
>> +private_lookup_attribute (const char *attr_name, size_t attr_len, tree list)
>> +{
>> +  while (list)
>> +    {
> 
>> @@ -151,17 +156,7 @@ lookup_attribute (const char *attr_name, tree list)
>>        /* Do the strlen() before calling the out-of-line implementation.
>>  	 In most cases attr_name is a string constant, and the compiler
>>  	 will optimize the strlen() away.  */
> 
> Part of the comment is of course here and that comment didn't make any sense
> when everything was inlined.

You're completely right.

> 
>> -      while (list)
>> -	{
>> -	  tree attr = get_attribute_name (list);
>> -	  size_t ident_len = IDENTIFIER_LENGTH (attr);
>> -	  if (cmp_attribs (attr_name, attr_len, IDENTIFIER_POINTER (attr),
>> -			   ident_len))
>> -	    break;
>> -	  list = TREE_CHAIN (list);
>> -	}
>> -
>> -      return list;
>> +      return private_lookup_attribute (attr_name, attr_len, list);
>>      }
>>  }
> 
> LGTM with the comment added.  In theory fnsplit could handle that too,
> but 1) it would emit out of line stuff in every TU separately 2) the
> compiler doesn't know that NULL DECL_ATTRIBUTES is so common (it could
> with profiledbootstrap).  And of course LTO can decide to inline it
> from attribs.c back anyway if there are reasons why it would be beneficial
> somewhere (but I doubt it is beneficial at least in most spots).

Doing it as it was is the right way ;)
Installed as r252022.

Martin

> 
> 	Jakub
>