| Message ID | 20170418151430.GZ1809@tucnak |
|---|---|
| State | New |
| Headers | show |
On 04/18/2017 09:14 AM, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on > ARRAY_TYPEs to denote types that need the special C++ alias handling. > The problem is how is that created, we just use build_array_type and > set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type > caching, so we might modify that way some other array type. > If all the array type creation goes through build_cplus_array_type, that > wouldn't be a problem, as that flag is dependent just on the element > type, but that is not the case, c-family as well as the middle-end has > lots of spots that also create array types. So in the end whether > one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on > GC etc. > > The following patch attempts to resolve this, by making the type hashing > take that flag into account. Bootstrapped/regtested on x86_64-linux and > i686-linux, ok for trunk? > > 2017-04-18 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/80423 > * tree.h (build_array_type_1): New prototype. > * tree.c (type_cache_hasher::equal): Also compare > TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs. > (build_array_type_1): No longer static. Add typeless_storage > argument, set TYPE_TYPELESS_STORAGE to it, if shared also > hash it, and pass to recursive call. > (build_array_type, build_nonshared_array_type): Adjust > build_array_type_1 callers. > c-family/ > * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE. > cp/ > * tree.c (build_cplus_array_type): Call build_array_type_1 > with the intended TYPE_TYPELESS_STORAGE flag value, instead > of calling build_array_type and modifying TYPE_TYPELESS_STORAGE > on the shared type. > testsuite/ > * g++.dg/other/pr80423.C: New test. Rather than exporting build_array_type_1, which seems rather gross, why not add a default argument for typeless storage to build_array_type prototype in tree.h? That way existing users "just work", and in the cases where we may want to specify typeless storage, we can pass it into the build_array_type wrapper. That seems marginally cleaner to me. jeff
On April 18, 2017 5:14:30 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on >ARRAY_TYPEs to denote types that need the special C++ alias handling. >The problem is how is that created, we just use build_array_type and >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type >caching, so we might modify that way some other array type. >If all the array type creation goes through build_cplus_array_type, >that >wouldn't be a problem, as that flag is dependent just on the element >type, but that is not the case, c-family as well as the middle-end has >lots of spots that also create array types. So in the end whether >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on >GC etc. > >The following patch attempts to resolve this, by making the type >hashing >take that flag into account. Bootstrapped/regtested on x86_64-linux >and >i686-linux, ok for trunk? When changing the C++ function I thought that calling build_array_type was wrong and it should instead do the same it does in the other places, use its raw creation routine and then the canonical type register stuff. But I was hesitant to change this at this point. I'm on FTO today so can't experiment with that before torrow. Richard. >2017-04-18 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/80423 > * tree.h (build_array_type_1): New prototype. > * tree.c (type_cache_hasher::equal): Also compare > TYPE_TYPELESS_STORAGE flag for ARRAY_TYPEs. > (build_array_type_1): No longer static. Add typeless_storage > argument, set TYPE_TYPELESS_STORAGE to it, if shared also > hash it, and pass to recursive call. > (build_array_type, build_nonshared_array_type): Adjust > build_array_type_1 callers. >c-family/ > * c-common.c (complete_array_type): Preserve TYPE_TYPELESS_STORAGE. >cp/ > * tree.c (build_cplus_array_type): Call build_array_type_1 > with the intended TYPE_TYPELESS_STORAGE flag value, instead > of calling build_array_type and modifying TYPE_TYPELESS_STORAGE > on the shared type. >testsuite/ > * g++.dg/other/pr80423.C: New test. > >--- gcc/tree.h.jj 2017-04-12 13:22:23.000000000 +0200 >+++ gcc/tree.h 2017-04-18 12:38:02.981708334 +0200 >@@ -4068,6 +4068,7 @@ extern tree build_truth_vector_type (uns > extern tree build_same_sized_truth_vector_type (tree vectype); > extern tree build_opaque_vector_type (tree innertype, int nunits); > extern tree build_index_type (tree); >+extern tree build_array_type_1 (tree, tree, bool, bool); > extern tree build_array_type (tree, tree); > extern tree build_nonshared_array_type (tree, tree); > extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT); >--- gcc/tree.c.jj 2017-04-07 11:46:46.000000000 +0200 >+++ gcc/tree.c 2017-04-18 12:37:38.765024350 +0200 >@@ -7073,7 +7073,9 @@ type_cache_hasher::equal (type_hash *a, > break; > return 0; > case ARRAY_TYPE: >- return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type); >+ return (TYPE_TYPELESS_STORAGE (a->type) >+ == TYPE_TYPELESS_STORAGE (b->type) >+ && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type)); > > case RECORD_TYPE: > case UNION_TYPE: >@@ -8352,8 +8354,9 @@ subrange_type_for_debug_p (const_tree ty > and number of elements specified by the range of values of INDEX_TYPE. >If SHARED is true, reuse such a type that has already been constructed. > */ > >-static tree >-build_array_type_1 (tree elt_type, tree index_type, bool shared) >+tree >+build_array_type_1 (tree elt_type, tree index_type, bool shared, >+ bool typeless_storage) > { > tree t; > >@@ -8367,6 +8370,7 @@ build_array_type_1 (tree elt_type, tree > TREE_TYPE (t) = elt_type; > TYPE_DOMAIN (t) = index_type; > TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type); >+ TYPE_TYPELESS_STORAGE (t) = typeless_storage; > layout_type (t); > > /* If the element type is incomplete at this point we get marked for >@@ -8381,6 +8385,7 @@ build_array_type_1 (tree elt_type, tree > hstate.add_object (TYPE_HASH (elt_type)); > if (index_type) > hstate.add_object (TYPE_HASH (index_type)); >+ hstate.add_flag (typeless_storage); > t = type_hash_canon (hstate.end (), t); > } > >@@ -8396,7 +8401,7 @@ build_array_type_1 (tree elt_type, tree > = build_array_type_1 (TYPE_CANONICAL (elt_type), > index_type > ? TYPE_CANONICAL (index_type) : NULL_TREE, >- shared); >+ shared, typeless_storage); > } > > return t; >@@ -8407,7 +8412,7 @@ build_array_type_1 (tree elt_type, tree > tree > build_array_type (tree elt_type, tree index_type) > { >- return build_array_type_1 (elt_type, index_type, true); >+ return build_array_type_1 (elt_type, index_type, true, false); > } > > /* Wrapper around build_array_type_1 with SHARED set to false. */ >@@ -8415,7 +8420,7 @@ build_array_type (tree elt_type, tree in > tree > build_nonshared_array_type (tree elt_type, tree index_type) > { >- return build_array_type_1 (elt_type, index_type, false); >+ return build_array_type_1 (elt_type, index_type, false, false); > } > > /* Return a representation of ELT_TYPE[NELTS], using indices of type >--- gcc/c-family/c-common.c.jj 2017-04-10 22:26:55.000000000 +0200 >+++ gcc/c-family/c-common.c 2017-04-18 14:43:53.448092585 +0200 >@@ -6397,7 +6397,6 @@ complete_array_type (tree *ptype, tree i > { > tree maxindex, type, main_type, elt, unqual_elt; > int failure = 0, quals; >- hashval_t hashcode = 0; > bool overflow_p = false; > > maxindex = size_zero_node; >@@ -6491,13 +6490,15 @@ complete_array_type (tree *ptype, tree i > TYPE_DOMAIN (main_type) > = build_range_type (TREE_TYPE (maxindex), > build_int_cst (TREE_TYPE (maxindex), 0), maxindex); >+ TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type); > layout_type (main_type); > > /* Make sure we have the canonical MAIN_TYPE. */ >- hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode); >- hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN >(main_type)), >- hashcode); >- main_type = type_hash_canon (hashcode, main_type); >+ inchash::hash hstate; >+ hstate.add_object (TYPE_HASH (unqual_elt)); >+ hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type))); >+ hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type)); >+ main_type = type_hash_canon (hstate.end (), main_type); > > /* Fix the canonical type. */ > if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type)) >@@ -6507,8 +6508,9 @@ complete_array_type (tree *ptype, tree i > || (TYPE_CANONICAL (TYPE_DOMAIN (main_type)) > != TYPE_DOMAIN (main_type))) > TYPE_CANONICAL (main_type) >- = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)), >- TYPE_CANONICAL (TYPE_DOMAIN (main_type))); >+ = build_array_type_1 (TYPE_CANONICAL (TREE_TYPE (main_type)), >+ TYPE_CANONICAL (TYPE_DOMAIN (main_type)), >+ true, TYPE_TYPELESS_STORAGE (main_type)); > else > TYPE_CANONICAL (main_type) = main_type; > >--- gcc/cp/tree.c.jj 2017-04-18 09:11:54.000000000 +0200 >+++ gcc/cp/tree.c 2017-04-18 14:42:28.911225896 +0200 >@@ -949,14 +949,14 @@ build_cplus_array_type (tree elt_type, t > } > else > { >- t = build_array_type (elt_type, index_type); >- if (elt_type == unsigned_char_type_node >- || elt_type == signed_char_type_node >- || elt_type == char_type_node >- || (TREE_CODE (elt_type) == ENUMERAL_TYPE >- && TYPE_CONTEXT (elt_type) == std_node >- && !strcmp ("byte", TYPE_NAME_STRING (elt_type)))) >- TYPE_TYPELESS_STORAGE (t) = 1; >+ bool typeless_storage >+ = (elt_type == unsigned_char_type_node >+ || elt_type == signed_char_type_node >+ || elt_type == char_type_node >+ || (TREE_CODE (elt_type) == ENUMERAL_TYPE >+ && TYPE_CONTEXT (elt_type) == std_node >+ && !strcmp ("byte", TYPE_NAME_STRING (elt_type)))); >+ t = build_array_type_1 (elt_type, index_type, true, >typeless_storage); > } > > /* Now check whether we already have this array variant. */ >--- gcc/testsuite/g++.dg/other/pr80423.C.jj 2017-04-18 >14:48:21.098510615 +0200 >+++ gcc/testsuite/g++.dg/other/pr80423.C 2017-04-18 14:49:02.049964200 >+0200 >@@ -0,0 +1,11 @@ >+// PR middle-end/80423 >+// { dg-do compile { target c++11 } } >+ >+typedef unsigned char uint8_t; > >+struct A { > >+ template <int N> A(unsigned char (&)[N]); > >+}; > >+void fn1(A) { > >+ uint8_t a[]{0}; > >+ fn1(a); > >+} > > > Jakub
On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote: > >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on > >ARRAY_TYPEs to denote types that need the special C++ alias handling. > >The problem is how is that created, we just use build_array_type and > >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses type > >caching, so we might modify that way some other array type. > >If all the array type creation goes through build_cplus_array_type, > >that > >wouldn't be a problem, as that flag is dependent just on the element > >type, but that is not the case, c-family as well as the middle-end has > >lots of spots that also create array types. So in the end whether > >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends on > >GC etc. > > > >The following patch attempts to resolve this, by making the type > >hashing > >take that flag into account. Bootstrapped/regtested on x86_64-linux > >and > >i686-linux, ok for trunk? > > When changing the C++ function I thought that calling build_array_type was > wrong and it should instead do the same it does in the other places, use > its raw creation routine and then the canonical type register stuff. But > I was hesitant to change this at this point. The problem is that as the patch shows, we don't need it in a single place (the C++ FE), but at least in two places (C++ FE and c-family), and it wouldn't surprise me if we don't need it later on in further places (e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say DSE wants to create a smaller one with the same property). Using a default argument to build_array_type is likely cleaner indeed, I'd just then also swap the arguments to build_array_type_1 (the shared vs. typeless_storage). Jakub
On April 19, 2017 7:56:30 AM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Wed, Apr 19, 2017 at 07:45:36AM +0200, Richard Biener wrote: >> >As mentioned in the PR, we now use TYPE_TYPELESS_STORAGE flag on >> >ARRAY_TYPEs to denote types that need the special C++ alias >handling. >> >The problem is how is that created, we just use build_array_type and >> >set TYPE_TYPELESS_STORAGE on the result, but build_array_type uses >type >> >caching, so we might modify that way some other array type. >> >If all the array type creation goes through build_cplus_array_type, >> >that >> >wouldn't be a problem, as that flag is dependent just on the element >> >type, but that is not the case, c-family as well as the middle-end >has >> >lots of spots that also create array types. So in the end whether >> >one gets TYPE_TYPELESS_STORAGE flag or not is quite random, depends >on >> >GC etc. >> > >> >The following patch attempts to resolve this, by making the type >> >hashing >> >take that flag into account. Bootstrapped/regtested on x86_64-linux >> >and >> >i686-linux, ok for trunk? >> >> When changing the C++ function I thought that calling >build_array_type was >> wrong and it should instead do the same it does in the other places, >use >> its raw creation routine and then the canonical type register stuff. >But >> I was hesitant to change this at this point. > >The problem is that as the patch shows, we don't need it in a single >place >(the C++ FE), but at least in two places (C++ FE and c-family), and it >wouldn't surprise me if we don't need it later on in further places >(e.g. in middle-end, if we have a TYPE_TYPELESS_STORAGE array and say >DSE >wants to create a smaller one with the same property). Hmm, but the C++ FE largely avoids the type cache of the Me and uses its own. Apart from a single place in this function. But as I said I'm AFK and can't really check. >Using a default argument to build_array_type is likely cleaner indeed, >I'd just then also swap the arguments to build_array_type_1 (the shared >vs. typeless_storage). Sure. With C++ there's no reason to export _1 variants. Either overloading or default args work for me here. Richard. > Jakub
--- gcc/tree.h.jj 2017-04-12 13:22:23.000000000 +0200 +++ gcc/tree.h 2017-04-18 12:38:02.981708334 +0200 @@ -4068,6 +4068,7 @@ extern tree build_truth_vector_type (uns extern tree build_same_sized_truth_vector_type (tree vectype); extern tree build_opaque_vector_type (tree innertype, int nunits); extern tree build_index_type (tree); +extern tree build_array_type_1 (tree, tree, bool, bool); extern tree build_array_type (tree, tree); extern tree build_nonshared_array_type (tree, tree); extern tree build_array_type_nelts (tree, unsigned HOST_WIDE_INT); --- gcc/tree.c.jj 2017-04-07 11:46:46.000000000 +0200 +++ gcc/tree.c 2017-04-18 12:37:38.765024350 +0200 @@ -7073,7 +7073,9 @@ type_cache_hasher::equal (type_hash *a, break; return 0; case ARRAY_TYPE: - return TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type); + return (TYPE_TYPELESS_STORAGE (a->type) + == TYPE_TYPELESS_STORAGE (b->type) + && TYPE_DOMAIN (a->type) == TYPE_DOMAIN (b->type)); case RECORD_TYPE: case UNION_TYPE: @@ -8352,8 +8354,9 @@ subrange_type_for_debug_p (const_tree ty and number of elements specified by the range of values of INDEX_TYPE. If SHARED is true, reuse such a type that has already been constructed. */ -static tree -build_array_type_1 (tree elt_type, tree index_type, bool shared) +tree +build_array_type_1 (tree elt_type, tree index_type, bool shared, + bool typeless_storage) { tree t; @@ -8367,6 +8370,7 @@ build_array_type_1 (tree elt_type, tree TREE_TYPE (t) = elt_type; TYPE_DOMAIN (t) = index_type; TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type); + TYPE_TYPELESS_STORAGE (t) = typeless_storage; layout_type (t); /* If the element type is incomplete at this point we get marked for @@ -8381,6 +8385,7 @@ build_array_type_1 (tree elt_type, tree hstate.add_object (TYPE_HASH (elt_type)); if (index_type) hstate.add_object (TYPE_HASH (index_type)); + hstate.add_flag (typeless_storage); t = type_hash_canon (hstate.end (), t); } @@ -8396,7 +8401,7 @@ build_array_type_1 (tree elt_type, tree = build_array_type_1 (TYPE_CANONICAL (elt_type), index_type ? TYPE_CANONICAL (index_type) : NULL_TREE, - shared); + shared, typeless_storage); } return t; @@ -8407,7 +8412,7 @@ build_array_type_1 (tree elt_type, tree tree build_array_type (tree elt_type, tree index_type) { - return build_array_type_1 (elt_type, index_type, true); + return build_array_type_1 (elt_type, index_type, true, false); } /* Wrapper around build_array_type_1 with SHARED set to false. */ @@ -8415,7 +8420,7 @@ build_array_type (tree elt_type, tree in tree build_nonshared_array_type (tree elt_type, tree index_type) { - return build_array_type_1 (elt_type, index_type, false); + return build_array_type_1 (elt_type, index_type, false, false); } /* Return a representation of ELT_TYPE[NELTS], using indices of type --- gcc/c-family/c-common.c.jj 2017-04-10 22:26:55.000000000 +0200 +++ gcc/c-family/c-common.c 2017-04-18 14:43:53.448092585 +0200 @@ -6397,7 +6397,6 @@ complete_array_type (tree *ptype, tree i { tree maxindex, type, main_type, elt, unqual_elt; int failure = 0, quals; - hashval_t hashcode = 0; bool overflow_p = false; maxindex = size_zero_node; @@ -6491,13 +6490,15 @@ complete_array_type (tree *ptype, tree i TYPE_DOMAIN (main_type) = build_range_type (TREE_TYPE (maxindex), build_int_cst (TREE_TYPE (maxindex), 0), maxindex); + TYPE_TYPELESS_STORAGE (main_type) = TYPE_TYPELESS_STORAGE (type); layout_type (main_type); /* Make sure we have the canonical MAIN_TYPE. */ - hashcode = iterative_hash_object (TYPE_HASH (unqual_elt), hashcode); - hashcode = iterative_hash_object (TYPE_HASH (TYPE_DOMAIN (main_type)), - hashcode); - main_type = type_hash_canon (hashcode, main_type); + inchash::hash hstate; + hstate.add_object (TYPE_HASH (unqual_elt)); + hstate.add_object (TYPE_HASH (TYPE_DOMAIN (main_type))); + hstate.add_flag (TYPE_TYPELESS_STORAGE (main_type)); + main_type = type_hash_canon (hstate.end (), main_type); /* Fix the canonical type. */ if (TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (main_type)) @@ -6507,8 +6508,9 @@ complete_array_type (tree *ptype, tree i || (TYPE_CANONICAL (TYPE_DOMAIN (main_type)) != TYPE_DOMAIN (main_type))) TYPE_CANONICAL (main_type) - = build_array_type (TYPE_CANONICAL (TREE_TYPE (main_type)), - TYPE_CANONICAL (TYPE_DOMAIN (main_type))); + = build_array_type_1 (TYPE_CANONICAL (TREE_TYPE (main_type)), + TYPE_CANONICAL (TYPE_DOMAIN (main_type)), + true, TYPE_TYPELESS_STORAGE (main_type)); else TYPE_CANONICAL (main_type) = main_type; --- gcc/cp/tree.c.jj 2017-04-18 09:11:54.000000000 +0200 +++ gcc/cp/tree.c 2017-04-18 14:42:28.911225896 +0200 @@ -949,14 +949,14 @@ build_cplus_array_type (tree elt_type, t } else { - t = build_array_type (elt_type, index_type); - if (elt_type == unsigned_char_type_node - || elt_type == signed_char_type_node - || elt_type == char_type_node - || (TREE_CODE (elt_type) == ENUMERAL_TYPE - && TYPE_CONTEXT (elt_type) == std_node - && !strcmp ("byte", TYPE_NAME_STRING (elt_type)))) - TYPE_TYPELESS_STORAGE (t) = 1; + bool typeless_storage + = (elt_type == unsigned_char_type_node + || elt_type == signed_char_type_node + || elt_type == char_type_node + || (TREE_CODE (elt_type) == ENUMERAL_TYPE + && TYPE_CONTEXT (elt_type) == std_node + && !strcmp ("byte", TYPE_NAME_STRING (elt_type)))); + t = build_array_type_1 (elt_type, index_type, true, typeless_storage); } /* Now check whether we already have this array variant. */ --- gcc/testsuite/g++.dg/other/pr80423.C.jj 2017-04-18 14:48:21.098510615 +0200 +++ gcc/testsuite/g++.dg/other/pr80423.C 2017-04-18 14:49:02.049964200 +0200 @@ -0,0 +1,11 @@ +// PR middle-end/80423 +// { dg-do compile { target c++11 } } + +typedef unsigned char uint8_t; +struct A { + template <int N> A(unsigned char (&)[N]); +}; +void fn1(A) { + uint8_t a[]{0}; + fn1(a); +}