| Message ID | 20170504144345.GS1809@tucnak |
|---|---|
| State | New |
| Headers | show |
On May 4, 2017 4:43:45 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >While type_hash_canon in case of reusing an already existing type >ggc_frees the freshly created type, we still waste one type uid >for each such case, this patch attempts to avoid that. >Furthermore, for INTEGER_TYPE we keep around the min and max value >INTEGER_CSTs and the cached values vector (until it is GCed). > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > >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. Richard. > return t1; > } > > Jakub
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, 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. Jakub
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
On Thu, May 04, 2017 at 06:21:17PM +0200, Richard Biener wrote: > >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. We don't, that crashes miserably. Jakub
On Thu, May 04, 2017 at 06:27:00PM +0200, Jakub Jelinek wrote: > On Thu, May 04, 2017 at 06:21:17PM +0200, Richard Biener wrote: > > >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. > > We don't, that crashes miserably. Tried that patch (moving the INTEGRAL_TYPE handling into free_node), and got 246 new FAILs in -flto testcases. Jakub
On Thu, 4 May 2017, Jakub Jelinek wrote: > On Thu, May 04, 2017 at 06:21:17PM +0200, Richard Biener wrote: > > >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. > > We don't, that crashes miserably. Ok, so then keep it in type_hash_canon (with a comment). Otherwise the patch is ok. Thanks, Richard.
--- 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); return t1; }