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
>From a40c06fc06afcb7bb886d7a3106e6da631a48430 Mon Sep 17 00:00:00 2001
From: marxin <[email protected]>
Date: Tue, 12 Sep 2017 13:30:39 +0200
Subject: [PATCH] Reduce lookup_attribute memory footprint.
gcc/ChangeLog:
2017-09-12 Martin Liska <[email protected]>
* 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
