This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Small type_hash_canon improvement
- From: Richard Biener <rguenther at suse dot de>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Thu, 04 May 2017 18:21:17 +0200
- Subject: Re: [PATCH] Small type_hash_canon improvement
- Authentication-results: sourceware.org; auth=none
- References: <20170504144345.GS1809@tucnak> <D3E8F88D-EE49-4277-AEFD-91C623F7CC1E@suse.de> <20170504160346.GU1809@tucnak>
On May 4, 2017 6:03:46 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Thu, May 04, 2017 at 05:54:47PM +0200, Richard Biener wrote:
>> >2017-05-04 Jakub Jelinek <jakub@redhat.com>
>> >
>> > * tree.c (next_type_uid): Change type to unsigned.
>> > (type_hash_canon): Decrement back next_type_uid if
>> > freeing a type node with the highest TYPE_UID. For INTEGER_TYPEs
>> > also ggc_free TYPE_MIN_VALUE, TYPE_MAX_VALUE and TYPE_CACHED_VALUES
>> > if possible.
>> >
>> >--- gcc/tree.c.jj 2017-05-03 16:55:39.688052581 +0200
>> >+++ gcc/tree.c 2017-05-03 18:49:30.662185944 +0200
>> >@@ -151,7 +151,7 @@ static const char * const tree_node_kind
>> > /* Unique id for next decl created. */
>> > static GTY(()) int next_decl_uid;
>> > /* Unique id for next type created. */
>> >-static GTY(()) int next_type_uid = 1;
>> >+static GTY(()) unsigned next_type_uid = 1;
>> > /* Unique id for next debug decl created. Use negative numbers,
>> > to catch erroneous uses. */
>> > static GTY(()) int next_debug_decl_uid;
>> >@@ -7188,6 +7188,19 @@ type_hash_canon (unsigned int hashcode,
>> > {
>> > tree t1 = ((type_hash *) *loc)->type;
>> > gcc_assert (TYPE_MAIN_VARIANT (t1) == t1);
>> >+ if (TYPE_UID (type) + 1 == next_type_uid)
>> >+ --next_type_uid;
>> >+ if (TREE_CODE (type) == INTEGER_TYPE)
>> >+ {
>> >+ if (TYPE_MIN_VALUE (type)
>> >+ && TREE_TYPE (TYPE_MIN_VALUE (type)) == type)
>> >+ ggc_free (TYPE_MIN_VALUE (type));
>> >+ if (TYPE_MAX_VALUE (type)
>> >+ && TREE_TYPE (TYPE_MAX_VALUE (type)) == type)
>> >+ ggc_free (TYPE_MAX_VALUE (type));
>> >+ if (TYPE_CACHED_VALUES_P (type))
>> >+ ggc_free (TYPE_CACHED_VALUES (type));
>> >+ }
>> > free_node (type);
>>
>> Shouldn't free_node handle this? That said, is freeing min/max safe?
> The constants are shared after all.
>
>The next_type_uid handling, I think it is better in type_hash_canon,
Agreed.
>the
>only other user after all calls free_node in a loop, so it is highly
>unlikely it would do anything there.
>
>If you mean the INTEGER_TYPE handling, then yes, I guess it could be
>done in free_node too and can move it there. If it was without
>the && TREE_TYPE (TYPE_M*_VALUE (type)) == type extra checks, then it
>is certainly unsafe and breaks bootstrap even, e.g. build_range_type
>and other spots happily create INTEGER_TYPEs with min/max value that
>have some other type. But when the type of the INTEGER_CSTs is the
>type we are ggc_freeing, anything that would refer to those constants
>afterwards would be necessarily broken (as their TREE_TYPE would be
>ggc_freed, possibly reused for something completely unrelated).
>Thus I think it should be safe even in the LTO case and thus doable
>in free_node.
OK. OTOH LTO frees the whole SCC and thus doesn't expect any pointed to stuff to be freed. Not sure if we allow double ggc_free of stuff.
Richard.
>
> Jakub