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

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

[-- Attachment #1: Type: text/plain, Size: 4235 bytes --]

On 09/12/2017 09:54 AM, Jakub Jelinek wrote:
> On Thu, Jul 13, 2017 at 03:51:31PM +0200, Martin Liška wrote:
>> It's request for comment where I mechanically moved attribute-related function to attribs.[hc].
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Thoughts?
> 
> I've only noticed this now, what is the reason for undoing the size
> optimization we had, i.e. inline lookup_attribute that handles the most
> common case inline only and inline computes the strlen (an important
> optimization, because we almost always call it with a string literal
> and strlen of that is a constant) and doing the rest out of line?
> 
>> -/* Given an attribute name ATTR_NAME and a list of attributes LIST,
>> -   return a pointer to the attribute's list element if the attribute
>> -   is part of the list, or NULL_TREE if not found.  If the attribute
>> -   appears more than once, this only returns the first occurrence; the
>> -   TREE_CHAIN of the return value should be passed back in if further
>> -   occurrences are wanted.  ATTR_NAME must be in the form 'text' (not
>> -   '__text__').  */
>> -
>> -static inline tree
>> -lookup_attribute (const char *attr_name, tree list)
>> -{
>> -  gcc_checking_assert (attr_name[0] != '_');  
>> -  /* In most cases, list is NULL_TREE.  */
>> -  if (list == NULL_TREE)
>> -    return NULL_TREE;
>> -  else
>> -    /* 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.  */
>> -    return private_lookup_attribute (attr_name, strlen (attr_name), list);
>> -}
> 
> The current code instead inlines the whole lookup_attribute which is larger.
> Have you looked at the code size effects of that change?
> 
> 	Jakub
> 

Hi.

Sorry for removal of the optimization. You're right it really adds some size (~30K of .text):

$ bloaty gcc/cc1plus -- /tmp/cc1plus 
     VM SIZE                      FILE SIZE
 ++++++++++++++ GROWING        ++++++++++++++
  [ = ]       0 .debug_loc      +277Ki  +0.4%
  [ = ]       0 .debug_info    +86.9Ki  +0.1%
  [ = ]       0 .debug_ranges  +59.9Ki  +0.5%
  +0.2% +32.1Ki .text          +32.1Ki  +0.2%
  [ = ]       0 .debug_line    +9.36Ki  +0.1%
  +0.0% +2.06Ki .rodata        +2.06Ki  +0.0%
  [ = ]       0 .symtab        +1.73Ki  +0.1%
  +0.1% +1.72Ki .eh_frame      +1.72Ki  +0.1%
  +0.0%    +272 .dynstr           +272  +0.0%
  [ = ]       0 .debug_abbrev     +140  +0.0%
  +0.0%     +72 .dynsym            +72  +0.0%
  +0.0%     +64 .eh_frame_hdr      +64  +0.0%
  [ = ]       0 .debug_aranges     +48  +0.0%
  +0.0%     +12 .gnu.hash          +12  +0.0%
  +0.0%     +12 .hash              +12  +0.0%
  +0.0%      +6 .gnu.version        +6  +0.0%

 -------------- SHRINKING      --------------
 -10.2%      -6 [Unmapped]        -317 -20.8%
  [ = ]       0 .strtab            -66  -0.0%
  [ = ]       0 .debug_str         -52  -0.0%

  +0.1% +36.3Ki TOTAL           +471Ki  +0.2%

$ bloaty gcc/cc1 -- /tmp/cc1
     VM SIZE                      FILE SIZE
 ++++++++++++++ GROWING        ++++++++++++++
  [ = ]       0 .debug_loc      +232Ki  +0.4%
  [ = ]       0 .debug_info    +74.1Ki  +0.1%
  [ = ]       0 .debug_ranges  +48.9Ki  +0.4%
  +0.2% +26.9Ki .text          +26.9Ki  +0.2%
  [ = ]       0 .debug_line    +8.22Ki  +0.1%
  +0.1% +1.79Ki .eh_frame      +1.79Ki  +0.1%
  +0.0% +1.75Ki .rodata        +1.75Ki  +0.0%
  [ = ]       0 .symtab        +1.50Ki  +0.1%
  +0.0%    +272 .dynstr           +272  +0.0%
  [ = ]       0 .debug_abbrev      +72  +0.0%
  +0.0%     +72 .dynsym            +72  +0.0%
  +0.0%     +64 .eh_frame_hdr      +64  +0.0%
  [ = ]       0 .debug_aranges     +48  +0.0%
  +0.0%     +12 .gnu.hash          +12  +0.0%
  +0.0%     +12 .hash              +12  +0.0%
  +0.0%      +6 .gnu.version        +6  +0.0%

 -------------- SHRINKING      --------------
  [ = ]       0 .debug_str         -52  -0.0%
  [ = ]       0 .strtab            -26  -0.0%

 -+-+-+-+-+-+-+ MIXED          +-+-+-+-+-+-+-
   +19%     +10 [Unmapped]     -2.87Ki -75.1%

  +0.1% +30.9Ki TOTAL           +392Ki  +0.2%

Thus I'm suggesting to the how it was. Is the patch OK after testing?

Martin

[-- Attachment #2: 0001-Reduce-lookup_attribute-memory-footprint.patch --]
[-- Type: text/x-patch, Size: 2453 bytes --]

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;
 }
+
+
+tree
+private_lookup_attribute (const char *attr_name, size_t attr_len, tree list)
+{
+  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;
+}
diff --git a/gcc/attribs.h b/gcc/attribs.h
index 06e6993e958..0285b93cbc3 100644
--- a/gcc/attribs.h
+++ b/gcc/attribs.h
@@ -87,6 +87,11 @@ extern tree handle_dll_attribute (tree *, tree, tree, int, bool *);
 extern int attribute_list_equal (const_tree, const_tree);
 extern int attribute_list_contained (const_tree, const_tree);
 
+/* The backbone of lookup_attribute().  ATTR_LEN is the string length
+   of ATTR_NAME, and LIST is not NULL_TREE.  */
+extern tree private_lookup_attribute (const char *attr_name, size_t attr_len,
+				      tree list);
+
 /* For a given IDENTIFIER_NODE, strip leading and trailing '_' characters
    so that we have a canonical form of attribute names.  */
 
@@ -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.  */
-      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);
     }
 }
 
-- 
2.14.1