| Message ID | 20190320230204.GQ7611@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | free_lang_data fixes (PR lto/89692) | expand |
On Thu, 21 Mar 2019, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, if e.g. build_aligned_type creates some specially > aligned variant of some TYPE_MAIN_VARIANT, that type doesn't appear in the > IL during free_lang_data and later on we call build_aligned_type with the > same arguments as before, we'll get the previously created aligned variant > which has not been free_lang_data processed and during LTO streaming we ICE > because of that. > > The following patch prunes unmarked types from the type variant chain, so > that we don't find them later on and instead create new type variants from > the free_lang_data processed types. > > Another change is just to be sure, marking TYPE_CANONICAL if it is not > already marked, so that we don't get weird behavior if we'd remove it. > In the usual case TYPE_CANONICAL will be the TYPE_MAIN_VARIANT type we mark > already anyway. > > The rest of the changes is to make sure the new types we create during > free_lang_data get marked in the pset, so that we will not try to purge > them. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list on the fld->pset.add () result? IMHO consistency should win over micro-optimizing the case you know the type will not be in the set. Thanks, Richard. > 2019-03-20 Jan Hubicka <hubicka@ucw.cz> > Jakub Jelinek <jakub@redhat.com> > > PR lto/89692 > * tree.c (fld_type_variant): Call fld->pset.add. > (fld_incomplete_type_of): Likewise and don't call add_tree_to_fld_list > if it returns true. > (fld_process_array_type): Likewise. > (free_lang_data_in_type): Purge non-marked types from TYPE_NEXT_VARIANT > list. > (find_decls_types_r): Call fld_worklist_push for TYPE_CANONICAL (t). > > * g++.dg/other/pr89692.C: New test. > > --- gcc/tree.c.jj 2019-03-20 12:24:56.443322228 +0100 > +++ gcc/tree.c 2019-03-20 20:16:12.277091827 +0100 > @@ -5215,6 +5215,7 @@ fld_type_variant (tree first, tree t, st > if (inner_type) > TREE_TYPE (v) = inner_type; > gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); > + fld->pset.add (v); > add_tree_to_fld_list (v, fld); > return v; > } > @@ -5253,7 +5254,8 @@ fld_process_array_type (tree t, tree t2, > array = build_array_type_1 (t2, TYPE_DOMAIN (t), > TYPE_TYPELESS_STORAGE (t), false); > TYPE_CANONICAL (array) = TYPE_CANONICAL (t); > - add_tree_to_fld_list (array, fld); > + if (!fld->pset.add (array)) > + add_tree_to_fld_list (array, fld); > } > return array; > } > @@ -5298,7 +5300,8 @@ fld_incomplete_type_of (tree t, struct f > TYPE_REF_CAN_ALIAS_ALL (t)); > gcc_assert (TYPE_CANONICAL (t2) != t2 > && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > - add_tree_to_fld_list (first, fld); > + if (!fld->pset.add (first)) > + add_tree_to_fld_list (first, fld); > return fld_type_variant (first, t, fld); > } > return t; > @@ -5321,6 +5324,7 @@ fld_incomplete_type_of (tree t, struct f > copy = build_distinct_type_copy (t); > > /* It is possible that type was not seen by free_lang_data yet. */ > + fld->pset.add (copy); > add_tree_to_fld_list (copy, fld); > TYPE_SIZE (copy) = NULL; > TYPE_USER_ALIGN (copy) = 0; > @@ -5445,6 +5449,18 @@ free_lang_data_in_type (tree type, struc > > TYPE_NEEDS_CONSTRUCTING (type) = 0; > > + /* Purge non-marked variants from the variants chain, so that they > + don't reappear in the IL after free_lang_data. */ > + while (TYPE_NEXT_VARIANT (type) > + && !fld->pset.contains (TYPE_NEXT_VARIANT (type))) > + { > + tree t = TYPE_NEXT_VARIANT (type); > + TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t); > + /* Turn the removed types into distinct types. */ > + TYPE_MAIN_VARIANT (t) = t; > + TYPE_NEXT_VARIANT (t) = NULL_TREE; > + } > + > if (TREE_CODE (type) == FUNCTION_TYPE) > { > TREE_TYPE (type) = fld_simplified_type (TREE_TYPE (type), fld); > @@ -5464,6 +5480,7 @@ free_lang_data_in_type (tree type, struc > & ~TYPE_QUAL_CONST > & ~TYPE_QUAL_VOLATILE; > TREE_VALUE (p) = build_qualified_type (arg_type, quals); > + fld->pset.add (TREE_VALUE (p)); > free_lang_data_in_type (TREE_VALUE (p), fld); > } > /* C++ FE uses TREE_PURPOSE to store initial values. */ > @@ -5886,8 +5903,7 @@ find_decls_types_r (tree *tp, int *ws, v > ctx = BLOCK_SUPERCONTEXT (ctx); > fld_worklist_push (ctx, fld); > } > - /* Do not walk TYPE_CANONICAL. We do not stream it and thus do not > - and want not to reach unused types this way. */ > + fld_worklist_push (TYPE_CANONICAL (t), fld); > > if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t)) > { > --- gcc/testsuite/g++.dg/other/pr89692.C.jj 2019-03-20 20:20:10.233278697 +0100 > +++ gcc/testsuite/g++.dg/other/pr89692.C 2019-03-20 20:19:53.193551778 +0100 > @@ -0,0 +1,20 @@ > +// PR lto/89692 > +// { dg-do compile } > +// { dg-require-effective-target lto } > +// { dg-options "-flto -O2" } > + > +struct S { > + short int a, b; > + unsigned char c : 1; > +}; > + > +bool > +foo (void) > +{ > + unsigned char d[sizeof (S)] = { 0 }; > + S e; > + > + __builtin_memcpy (&e, d, sizeof (d)); > + > + return e.c == d[0]; > +} > > Jakub >
On Thu, Mar 21, 2019 at 09:35:07AM +0100, Richard Biener wrote: > LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list > on the fld->pset.add () result? IMHO consistency should win over > micro-optimizing the case you know the type will not be in the set. Yeah, it was a optimization for the most common case, when find_decls_types_r calls add_tree_to_fld_list (t, fld); for DECL_P as well as TYPE_P, because in that case the fld->pset.add check has been done already by walk_tree. If we have hundreds thousands of elts in the hash_set, it could be more than a microoptimization, but if you think strongly that it should be all thrown away and add_tree_to_fld_list should start with if (fld->pset.add (t)) return; then I can do that too. > > 2019-03-20 Jan Hubicka <hubicka@ucw.cz> > > Jakub Jelinek <jakub@redhat.com> > > > > PR lto/89692 > > * tree.c (fld_type_variant): Call fld->pset.add. > > (fld_incomplete_type_of): Likewise and don't call add_tree_to_fld_list > > if it returns true. > > (fld_process_array_type): Likewise. > > (free_lang_data_in_type): Purge non-marked types from TYPE_NEXT_VARIANT > > list. > > (find_decls_types_r): Call fld_worklist_push for TYPE_CANONICAL (t). > > > > * g++.dg/other/pr89692.C: New test. Jakub
On Thu, 21 Mar 2019, Jakub Jelinek wrote: > On Thu, Mar 21, 2019 at 09:35:07AM +0100, Richard Biener wrote: > > LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list > > on the fld->pset.add () result? IMHO consistency should win over > > micro-optimizing the case you know the type will not be in the set. > > Yeah, it was a optimization for the most common case, when > find_decls_types_r calls add_tree_to_fld_list (t, fld); for DECL_P as well > as TYPE_P, because in that case the fld->pset.add check has been done > already by walk_tree. If we have hundreds thousands of elts in the > hash_set, it could be more than a microoptimization, but if you think > strongly that it should be all thrown away and add_tree_to_fld_list should > start with if (fld->pset.add (t)) return; then I can do that too. Yes, I think that would be better. Richard.
On Thu, Mar 21, 2019 at 10:11:00AM +0100, Richard Biener wrote: > On Thu, 21 Mar 2019, Jakub Jelinek wrote: > > > On Thu, Mar 21, 2019 at 09:35:07AM +0100, Richard Biener wrote: > > > LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list > > > on the fld->pset.add () result? IMHO consistency should win over > > > micro-optimizing the case you know the type will not be in the set. > > > > Yeah, it was a optimization for the most common case, when > > find_decls_types_r calls add_tree_to_fld_list (t, fld); for DECL_P as well > > as TYPE_P, because in that case the fld->pset.add check has been done > > already by walk_tree. If we have hundreds thousands of elts in the > > hash_set, it could be more than a microoptimization, but if you think > > strongly that it should be all thrown away and add_tree_to_fld_list should > > start with if (fld->pset.add (t)) return; then I can do that too. > > Yes, I think that would be better. Actually sorry, that is not going to work, because in the two find_decls_types_r cases it is already set in the pset by walk_tree and thus fld->pset.add (t) will return true because it is the second addition. And just doing fld->pset.add (t) and not testing the result is undesirable too for the other cases; in the patch there was some set of cases where I was sure that the type isn't already in the pset (e.g. immediately after build_variant_type_copy or build_distinct_type_copy), but in several other cases it is unclear if it is already in or not and I think we don't want to add it again in those cases. Would you prefer to have say add_tree_to_fld_list that does what it does now and has just 2 callers in find_decls_types_r and a new maybe_add_tree_to_fld_list that would do if (!fld->pset.add (t)) add_tree_to_fld_list (t, fld); and change all the other add_tree_to_fld_list callers? Jakub
On Thu, 21 Mar 2019, Jakub Jelinek wrote: > On Thu, Mar 21, 2019 at 10:11:00AM +0100, Richard Biener wrote: > > On Thu, 21 Mar 2019, Jakub Jelinek wrote: > > > > > On Thu, Mar 21, 2019 at 09:35:07AM +0100, Richard Biener wrote: > > > > LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list > > > > on the fld->pset.add () result? IMHO consistency should win over > > > > micro-optimizing the case you know the type will not be in the set. > > > > > > Yeah, it was a optimization for the most common case, when > > > find_decls_types_r calls add_tree_to_fld_list (t, fld); for DECL_P as well > > > as TYPE_P, because in that case the fld->pset.add check has been done > > > already by walk_tree. If we have hundreds thousands of elts in the > > > hash_set, it could be more than a microoptimization, but if you think > > > strongly that it should be all thrown away and add_tree_to_fld_list should > > > start with if (fld->pset.add (t)) return; then I can do that too. > > > > Yes, I think that would be better. > > Actually sorry, that is not going to work, because in the two > find_decls_types_r cases it is already set in the pset by walk_tree and thus > fld->pset.add (t) will return true because it is the second addition. Oh. I was mostly looking at the cases you add, find_decls_types_r is indeed special. > And just doing fld->pset.add (t) and not testing the result is undesirable > too for the other cases; in the patch there was some set of cases where I > was sure that the type isn't already in the pset (e.g. immediately after > build_variant_type_copy or build_distinct_type_copy), but in several other > cases it is unclear if it is already in or not and I think we don't want to > add it again in those cases. Yeah, I think there's no point to add it again. Or is there a case I am missing - Honza? > Would you prefer to have say add_tree_to_fld_list that does what it does now > and has just 2 callers in find_decls_types_r and a new > maybe_add_tree_to_fld_list that would do > if (!fld->pset.add (t)) > add_tree_to_fld_list (t, fld); > and change all the other add_tree_to_fld_list callers? Not sure if that's needed, if you leave find_decls_types_r alone then always doing the check should work, no? Richard.
On Thu, Mar 21, 2019 at 11:13:41AM +0100, Richard Biener wrote: > > Would you prefer to have say add_tree_to_fld_list that does what it does now > > and has just 2 callers in find_decls_types_r and a new > > maybe_add_tree_to_fld_list that would do > > if (!fld->pset.add (t)) > > add_tree_to_fld_list (t, fld); > > and change all the other add_tree_to_fld_list callers? > > Not sure if that's needed, if you leave find_decls_types_r alone then > always doing the check should work, no? Well, find_decls_types_r uses add_tree_to_fld_list, so the if (fld->pset.add (t)) return; can't be in that function. If we want to simplify all the other callers, so that they don't need to do it manually, then the only options I see is call some other function in those spots that does this additional check, or call another function that doesn't do that in the two spots of find_decls_types_r, or add another argument to the function say with default argument and test that argument before the fld->pset.add. E.g. , bool do_add = true and if (do_add && fld->pset.add (t)) return; and adjust the two calls in find_decls_types_r to call add_tree_to_fld_list (t, fld, false); Or keep the patch as is, do the tests before each but the two add_tree_to_fld_list calls. Or do you have yet another variant, or what from the above do you prefer? Jakub
On Thu, 21 Mar 2019, Jakub Jelinek wrote: > On Thu, Mar 21, 2019 at 11:13:41AM +0100, Richard Biener wrote: > > > Would you prefer to have say add_tree_to_fld_list that does what it does now > > > and has just 2 callers in find_decls_types_r and a new > > > maybe_add_tree_to_fld_list that would do > > > if (!fld->pset.add (t)) > > > add_tree_to_fld_list (t, fld); > > > and change all the other add_tree_to_fld_list callers? > > > > Not sure if that's needed, if you leave find_decls_types_r alone then > > always doing the check should work, no? > > Well, find_decls_types_r uses add_tree_to_fld_list, so the if (fld->pset.add > (t)) return; can't be in that function. If we want to simplify all the > other callers, so that they don't need to do it manually, then the only > options I see is call some other function in those spots that does this > additional check, or call another function that doesn't do that in the > two spots of find_decls_types_r, or add another argument to the function > say with default argument and test that argument before the fld->pset.add. > E.g. , bool do_add = true and if (do_add && fld->pset.add (t)) return; > and adjust the two calls in find_decls_types_r to call > add_tree_to_fld_list (t, fld, false); > Or keep the patch as is, do the tests before each but the two > add_tree_to_fld_list calls. I'd have kept the patch but made the add_tree_to_fld_list following the new fld->pset.add (t) calls conditional on the result. Richard.
On Thu, Mar 21, 2019 at 11:25:38AM +0100, Richard Biener wrote: > On Thu, 21 Mar 2019, Jakub Jelinek wrote: > > > On Thu, Mar 21, 2019 at 11:13:41AM +0100, Richard Biener wrote: > > > > Would you prefer to have say add_tree_to_fld_list that does what it does now > > > > and has just 2 callers in find_decls_types_r and a new > > > > maybe_add_tree_to_fld_list that would do > > > > if (!fld->pset.add (t)) > > > > add_tree_to_fld_list (t, fld); > > > > and change all the other add_tree_to_fld_list callers? > > > > > > Not sure if that's needed, if you leave find_decls_types_r alone then > > > always doing the check should work, no? > > > > Well, find_decls_types_r uses add_tree_to_fld_list, so the if (fld->pset.add > > (t)) return; can't be in that function. If we want to simplify all the > > other callers, so that they don't need to do it manually, then the only > > options I see is call some other function in those spots that does this > > additional check, or call another function that doesn't do that in the > > two spots of find_decls_types_r, or add another argument to the function > > say with default argument and test that argument before the fld->pset.add. > > E.g. , bool do_add = true and if (do_add && fld->pset.add (t)) return; > > and adjust the two calls in find_decls_types_r to call > > add_tree_to_fld_list (t, fld, false); > > Or keep the patch as is, do the tests before each but the two > > add_tree_to_fld_list calls. > > I'd have kept the patch but made the add_tree_to_fld_list following > the new fld->pset.add (t) calls conditional on the result. Ok, so incremental diff: --- gcc/tree.c 2019-03-20 20:16:12.277091827 +0100 +++ gcc/tree.c 2019-03-21 11:33:17.008936452 +0100 @@ -5215,8 +5215,8 @@ if (inner_type) TREE_TYPE (v) = inner_type; gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); - fld->pset.add (v); - add_tree_to_fld_list (v, fld); + if (!fld->pset.add (v)) + add_tree_to_fld_list (v, fld); return v; } @@ -5324,8 +5324,8 @@ copy = build_distinct_type_copy (t); /* It is possible that type was not seen by free_lang_data yet. */ - fld->pset.add (copy); - add_tree_to_fld_list (copy, fld); + if (!fld->pset.add (copy)) + add_tree_to_fld_list (copy, fld); TYPE_SIZE (copy) = NULL; TYPE_USER_ALIGN (copy) = 0; TYPE_SIZE_UNIT (copy) = NULL; @@ -5480,8 +5480,8 @@ & ~TYPE_QUAL_CONST & ~TYPE_QUAL_VOLATILE; TREE_VALUE (p) = build_qualified_type (arg_type, quals); - fld->pset.add (TREE_VALUE (p)); - free_lang_data_in_type (TREE_VALUE (p), fld); + if (!fld->pset.add (TREE_VALUE (p))) + free_lang_data_in_type (TREE_VALUE (p), fld); } /* C++ FE uses TREE_PURPOSE to store initial values. */ TREE_PURPOSE (p) = NULL; and full patch below? Ok for trunk if it passes bootstrap/regtest? 2019-03-21 Jan Hubicka <hubicka@ucw.cz> Jakub Jelinek <jakub@redhat.com> PR lto/89692 * tree.c (fld_type_variant, fld_incomplete_type_of, fld_process_array_type): Call fld->pset.add and don't call add_tree_to_fld_list if it returns true. (free_lang_data_in_type): Likewise. Purge non-marked types from TYPE_NEXT_VARIANT list. (find_decls_types_r): Call fld_worklist_push for TYPE_CANONICAL (t). * g++.dg/other/pr89692.C: New test. --- gcc/tree.c.jj 2019-03-20 12:24:56.443322228 +0100 +++ gcc/tree.c 2019-03-21 11:33:17.008936452 +0100 @@ -5215,7 +5215,8 @@ fld_type_variant (tree first, tree t, st if (inner_type) TREE_TYPE (v) = inner_type; gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); - add_tree_to_fld_list (v, fld); + if (!fld->pset.add (v)) + add_tree_to_fld_list (v, fld); return v; } @@ -5253,7 +5254,8 @@ fld_process_array_type (tree t, tree t2, array = build_array_type_1 (t2, TYPE_DOMAIN (t), TYPE_TYPELESS_STORAGE (t), false); TYPE_CANONICAL (array) = TYPE_CANONICAL (t); - add_tree_to_fld_list (array, fld); + if (!fld->pset.add (array)) + add_tree_to_fld_list (array, fld); } return array; } @@ -5298,7 +5300,8 @@ fld_incomplete_type_of (tree t, struct f TYPE_REF_CAN_ALIAS_ALL (t)); gcc_assert (TYPE_CANONICAL (t2) != t2 && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); - add_tree_to_fld_list (first, fld); + if (!fld->pset.add (first)) + add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); } return t; @@ -5321,7 +5324,8 @@ fld_incomplete_type_of (tree t, struct f copy = build_distinct_type_copy (t); /* It is possible that type was not seen by free_lang_data yet. */ - add_tree_to_fld_list (copy, fld); + if (!fld->pset.add (copy)) + add_tree_to_fld_list (copy, fld); TYPE_SIZE (copy) = NULL; TYPE_USER_ALIGN (copy) = 0; TYPE_SIZE_UNIT (copy) = NULL; @@ -5445,6 +5449,18 @@ free_lang_data_in_type (tree type, struc TYPE_NEEDS_CONSTRUCTING (type) = 0; + /* Purge non-marked variants from the variants chain, so that they + don't reappear in the IL after free_lang_data. */ + while (TYPE_NEXT_VARIANT (type) + && !fld->pset.contains (TYPE_NEXT_VARIANT (type))) + { + tree t = TYPE_NEXT_VARIANT (type); + TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t); + /* Turn the removed types into distinct types. */ + TYPE_MAIN_VARIANT (t) = t; + TYPE_NEXT_VARIANT (t) = NULL_TREE; + } + if (TREE_CODE (type) == FUNCTION_TYPE) { TREE_TYPE (type) = fld_simplified_type (TREE_TYPE (type), fld); @@ -5464,7 +5480,8 @@ free_lang_data_in_type (tree type, struc & ~TYPE_QUAL_CONST & ~TYPE_QUAL_VOLATILE; TREE_VALUE (p) = build_qualified_type (arg_type, quals); - free_lang_data_in_type (TREE_VALUE (p), fld); + if (!fld->pset.add (TREE_VALUE (p))) + free_lang_data_in_type (TREE_VALUE (p), fld); } /* C++ FE uses TREE_PURPOSE to store initial values. */ TREE_PURPOSE (p) = NULL; @@ -5886,8 +5903,7 @@ find_decls_types_r (tree *tp, int *ws, v ctx = BLOCK_SUPERCONTEXT (ctx); fld_worklist_push (ctx, fld); } - /* Do not walk TYPE_CANONICAL. We do not stream it and thus do not - and want not to reach unused types this way. */ + fld_worklist_push (TYPE_CANONICAL (t), fld); if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t)) { --- gcc/testsuite/g++.dg/other/pr89692.C.jj 2019-03-20 20:20:10.233278697 +0100 +++ gcc/testsuite/g++.dg/other/pr89692.C 2019-03-20 20:19:53.193551778 +0100 @@ -0,0 +1,20 @@ +// PR lto/89692 +// { dg-do compile } +// { dg-require-effective-target lto } +// { dg-options "-flto -O2" } + +struct S { + short int a, b; + unsigned char c : 1; +}; + +bool +foo (void) +{ + unsigned char d[sizeof (S)] = { 0 }; + S e; + + __builtin_memcpy (&e, d, sizeof (d)); + + return e.c == d[0]; +} Jakub
On Thu, 21 Mar 2019, Jakub Jelinek wrote: > On Thu, Mar 21, 2019 at 11:25:38AM +0100, Richard Biener wrote: > > On Thu, 21 Mar 2019, Jakub Jelinek wrote: > > > > > On Thu, Mar 21, 2019 at 11:13:41AM +0100, Richard Biener wrote: > > > > > Would you prefer to have say add_tree_to_fld_list that does what it does now > > > > > and has just 2 callers in find_decls_types_r and a new > > > > > maybe_add_tree_to_fld_list that would do > > > > > if (!fld->pset.add (t)) > > > > > add_tree_to_fld_list (t, fld); > > > > > and change all the other add_tree_to_fld_list callers? > > > > > > > > Not sure if that's needed, if you leave find_decls_types_r alone then > > > > always doing the check should work, no? > > > > > > Well, find_decls_types_r uses add_tree_to_fld_list, so the if (fld->pset.add > > > (t)) return; can't be in that function. If we want to simplify all the > > > other callers, so that they don't need to do it manually, then the only > > > options I see is call some other function in those spots that does this > > > additional check, or call another function that doesn't do that in the > > > two spots of find_decls_types_r, or add another argument to the function > > > say with default argument and test that argument before the fld->pset.add. > > > E.g. , bool do_add = true and if (do_add && fld->pset.add (t)) return; > > > and adjust the two calls in find_decls_types_r to call > > > add_tree_to_fld_list (t, fld, false); > > > Or keep the patch as is, do the tests before each but the two > > > add_tree_to_fld_list calls. > > > > I'd have kept the patch but made the add_tree_to_fld_list following > > the new fld->pset.add (t) calls conditional on the result. > > Ok, so incremental diff: > --- gcc/tree.c 2019-03-20 20:16:12.277091827 +0100 > +++ gcc/tree.c 2019-03-21 11:33:17.008936452 +0100 > @@ -5215,8 +5215,8 @@ > if (inner_type) > TREE_TYPE (v) = inner_type; > gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); > - fld->pset.add (v); > - add_tree_to_fld_list (v, fld); > + if (!fld->pset.add (v)) > + add_tree_to_fld_list (v, fld); > return v; > } > > @@ -5324,8 +5324,8 @@ > copy = build_distinct_type_copy (t); > > /* It is possible that type was not seen by free_lang_data yet. */ > - fld->pset.add (copy); > - add_tree_to_fld_list (copy, fld); > + if (!fld->pset.add (copy)) > + add_tree_to_fld_list (copy, fld); > TYPE_SIZE (copy) = NULL; > TYPE_USER_ALIGN (copy) = 0; > TYPE_SIZE_UNIT (copy) = NULL; > @@ -5480,8 +5480,8 @@ > & ~TYPE_QUAL_CONST > & ~TYPE_QUAL_VOLATILE; > TREE_VALUE (p) = build_qualified_type (arg_type, quals); > - fld->pset.add (TREE_VALUE (p)); > - free_lang_data_in_type (TREE_VALUE (p), fld); > + if (!fld->pset.add (TREE_VALUE (p))) > + free_lang_data_in_type (TREE_VALUE (p), fld); > } > /* C++ FE uses TREE_PURPOSE to store initial values. */ > TREE_PURPOSE (p) = NULL; > > and full patch below? Ok for trunk if it passes bootstrap/regtest? OK. Thanks, Richard. > 2019-03-21 Jan Hubicka <hubicka@ucw.cz> > Jakub Jelinek <jakub@redhat.com> > > PR lto/89692 > * tree.c (fld_type_variant, fld_incomplete_type_of, > fld_process_array_type): Call fld->pset.add and don't call > add_tree_to_fld_list if it returns true. > (free_lang_data_in_type): Likewise. Purge non-marked types from > TYPE_NEXT_VARIANT list. > (find_decls_types_r): Call fld_worklist_push for TYPE_CANONICAL (t). > > * g++.dg/other/pr89692.C: New test. > > --- gcc/tree.c.jj 2019-03-20 12:24:56.443322228 +0100 > +++ gcc/tree.c 2019-03-21 11:33:17.008936452 +0100 > @@ -5215,7 +5215,8 @@ fld_type_variant (tree first, tree t, st > if (inner_type) > TREE_TYPE (v) = inner_type; > gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); > - add_tree_to_fld_list (v, fld); > + if (!fld->pset.add (v)) > + add_tree_to_fld_list (v, fld); > return v; > } > > @@ -5253,7 +5254,8 @@ fld_process_array_type (tree t, tree t2, > array = build_array_type_1 (t2, TYPE_DOMAIN (t), > TYPE_TYPELESS_STORAGE (t), false); > TYPE_CANONICAL (array) = TYPE_CANONICAL (t); > - add_tree_to_fld_list (array, fld); > + if (!fld->pset.add (array)) > + add_tree_to_fld_list (array, fld); > } > return array; > } > @@ -5298,7 +5300,8 @@ fld_incomplete_type_of (tree t, struct f > TYPE_REF_CAN_ALIAS_ALL (t)); > gcc_assert (TYPE_CANONICAL (t2) != t2 > && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); > - add_tree_to_fld_list (first, fld); > + if (!fld->pset.add (first)) > + add_tree_to_fld_list (first, fld); > return fld_type_variant (first, t, fld); > } > return t; > @@ -5321,7 +5324,8 @@ fld_incomplete_type_of (tree t, struct f > copy = build_distinct_type_copy (t); > > /* It is possible that type was not seen by free_lang_data yet. */ > - add_tree_to_fld_list (copy, fld); > + if (!fld->pset.add (copy)) > + add_tree_to_fld_list (copy, fld); > TYPE_SIZE (copy) = NULL; > TYPE_USER_ALIGN (copy) = 0; > TYPE_SIZE_UNIT (copy) = NULL; > @@ -5445,6 +5449,18 @@ free_lang_data_in_type (tree type, struc > > TYPE_NEEDS_CONSTRUCTING (type) = 0; > > + /* Purge non-marked variants from the variants chain, so that they > + don't reappear in the IL after free_lang_data. */ > + while (TYPE_NEXT_VARIANT (type) > + && !fld->pset.contains (TYPE_NEXT_VARIANT (type))) > + { > + tree t = TYPE_NEXT_VARIANT (type); > + TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t); > + /* Turn the removed types into distinct types. */ > + TYPE_MAIN_VARIANT (t) = t; > + TYPE_NEXT_VARIANT (t) = NULL_TREE; > + } > + > if (TREE_CODE (type) == FUNCTION_TYPE) > { > TREE_TYPE (type) = fld_simplified_type (TREE_TYPE (type), fld); > @@ -5464,7 +5480,8 @@ free_lang_data_in_type (tree type, struc > & ~TYPE_QUAL_CONST > & ~TYPE_QUAL_VOLATILE; > TREE_VALUE (p) = build_qualified_type (arg_type, quals); > - free_lang_data_in_type (TREE_VALUE (p), fld); > + if (!fld->pset.add (TREE_VALUE (p))) > + free_lang_data_in_type (TREE_VALUE (p), fld); > } > /* C++ FE uses TREE_PURPOSE to store initial values. */ > TREE_PURPOSE (p) = NULL; > @@ -5886,8 +5903,7 @@ find_decls_types_r (tree *tp, int *ws, v > ctx = BLOCK_SUPERCONTEXT (ctx); > fld_worklist_push (ctx, fld); > } > - /* Do not walk TYPE_CANONICAL. We do not stream it and thus do not > - and want not to reach unused types this way. */ > + fld_worklist_push (TYPE_CANONICAL (t), fld); > > if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t)) > { > --- gcc/testsuite/g++.dg/other/pr89692.C.jj 2019-03-20 20:20:10.233278697 +0100 > +++ gcc/testsuite/g++.dg/other/pr89692.C 2019-03-20 20:19:53.193551778 +0100 > @@ -0,0 +1,20 @@ > +// PR lto/89692 > +// { dg-do compile } > +// { dg-require-effective-target lto } > +// { dg-options "-flto -O2" } > + > +struct S { > + short int a, b; > + unsigned char c : 1; > +}; > + > +bool > +foo (void) > +{ > + unsigned char d[sizeof (S)] = { 0 }; > + S e; > + > + __builtin_memcpy (&e, d, sizeof (d)); > + > + return e.c == d[0]; > +} > > > Jakub >
--- gcc/tree.c.jj 2019-03-20 12:24:56.443322228 +0100 +++ gcc/tree.c 2019-03-20 20:16:12.277091827 +0100 @@ -5215,6 +5215,7 @@ fld_type_variant (tree first, tree t, st if (inner_type) TREE_TYPE (v) = inner_type; gcc_checking_assert (fld_type_variant_equal_p (t,v, inner_type)); + fld->pset.add (v); add_tree_to_fld_list (v, fld); return v; } @@ -5253,7 +5254,8 @@ fld_process_array_type (tree t, tree t2, array = build_array_type_1 (t2, TYPE_DOMAIN (t), TYPE_TYPELESS_STORAGE (t), false); TYPE_CANONICAL (array) = TYPE_CANONICAL (t); - add_tree_to_fld_list (array, fld); + if (!fld->pset.add (array)) + add_tree_to_fld_list (array, fld); } return array; } @@ -5298,7 +5300,8 @@ fld_incomplete_type_of (tree t, struct f TYPE_REF_CAN_ALIAS_ALL (t)); gcc_assert (TYPE_CANONICAL (t2) != t2 && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))); - add_tree_to_fld_list (first, fld); + if (!fld->pset.add (first)) + add_tree_to_fld_list (first, fld); return fld_type_variant (first, t, fld); } return t; @@ -5321,6 +5324,7 @@ fld_incomplete_type_of (tree t, struct f copy = build_distinct_type_copy (t); /* It is possible that type was not seen by free_lang_data yet. */ + fld->pset.add (copy); add_tree_to_fld_list (copy, fld); TYPE_SIZE (copy) = NULL; TYPE_USER_ALIGN (copy) = 0; @@ -5445,6 +5449,18 @@ free_lang_data_in_type (tree type, struc TYPE_NEEDS_CONSTRUCTING (type) = 0; + /* Purge non-marked variants from the variants chain, so that they + don't reappear in the IL after free_lang_data. */ + while (TYPE_NEXT_VARIANT (type) + && !fld->pset.contains (TYPE_NEXT_VARIANT (type))) + { + tree t = TYPE_NEXT_VARIANT (type); + TYPE_NEXT_VARIANT (type) = TYPE_NEXT_VARIANT (t); + /* Turn the removed types into distinct types. */ + TYPE_MAIN_VARIANT (t) = t; + TYPE_NEXT_VARIANT (t) = NULL_TREE; + } + if (TREE_CODE (type) == FUNCTION_TYPE) { TREE_TYPE (type) = fld_simplified_type (TREE_TYPE (type), fld); @@ -5464,6 +5480,7 @@ free_lang_data_in_type (tree type, struc & ~TYPE_QUAL_CONST & ~TYPE_QUAL_VOLATILE; TREE_VALUE (p) = build_qualified_type (arg_type, quals); + fld->pset.add (TREE_VALUE (p)); free_lang_data_in_type (TREE_VALUE (p), fld); } /* C++ FE uses TREE_PURPOSE to store initial values. */ @@ -5886,8 +5903,7 @@ find_decls_types_r (tree *tp, int *ws, v ctx = BLOCK_SUPERCONTEXT (ctx); fld_worklist_push (ctx, fld); } - /* Do not walk TYPE_CANONICAL. We do not stream it and thus do not - and want not to reach unused types this way. */ + fld_worklist_push (TYPE_CANONICAL (t), fld); if (RECORD_OR_UNION_TYPE_P (t) && TYPE_BINFO (t)) { --- gcc/testsuite/g++.dg/other/pr89692.C.jj 2019-03-20 20:20:10.233278697 +0100 +++ gcc/testsuite/g++.dg/other/pr89692.C 2019-03-20 20:19:53.193551778 +0100 @@ -0,0 +1,20 @@ +// PR lto/89692 +// { dg-do compile } +// { dg-require-effective-target lto } +// { dg-options "-flto -O2" } + +struct S { + short int a, b; + unsigned char c : 1; +}; + +bool +foo (void) +{ + unsigned char d[sizeof (S)] = { 0 }; + S e; + + __builtin_memcpy (&e, d, sizeof (d)); + + return e.c == d[0]; +}