| Message ID | f93202b6-dc3e-b41d-29e8-5182cc1ce3a7@acm.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 3, 2017 at 9:03 PM, Nathan Sidwell <nathan@acm.org> wrote: > Bill, > can you give this patch a spin please? I've smoke tested it on a ppc64le > x-compiler, but don't have one to run executables. regression testing on an > x86_64-linux system is ok. > > The DEPENDENT_TYPE_VALID_P thing is a red herring. > > It is the canonical type table's equal function considering most types with > different TYPE_NAMEs to be different. Now, you might think that only > applies to things like RECORD_TYPE, but no, wchar_t is different from plain > int, for example. > > However, as you can see there's already a get-out for COMPLEX_TYPE, and I > think the same is needed for VECTOR_TYPE. Hmm, but that is essentially a hack given that build_complex_type does things it shouldn't (assign a name to the type). make_vector_type doesn't do that so it shouldn't get such special-handing. > Both set_underlying_type and rs6000 set the builtin vector type's TYPE_NAME, > and so one constructed via applying __attribute__((vector_size(16))) will > never match. And it should. why? They only need to share the canonical type not the type node itself (unless their name is the same, of course). Now -- that name comparing is somewhat odd. We hash type "variants" with different names the same (so setting the name after inserting sth into the hash is allowed, but it will overwrite the old entry so any unnamed uses up to now get a type with a name...). I guess we'd be better off leaving only unnamed types in the hash and building a type variant whenever we add a name to such type. Richard. > nathan > -- > Nathan Sidwell >
On 04/04/2017 04:27 AM, Richard Biener wrote: > On Mon, Apr 3, 2017 at 9:03 PM, Nathan Sidwell <nathan@acm.org> wrote: >> However, as you can see there's already a get-out for COMPLEX_TYPE, and I >> think the same is needed for VECTOR_TYPE. > > Hmm, but that is essentially a hack given that build_complex_type does things > it shouldn't (assign a name to the type). Oh, didn't know that. >> Both set_underlying_type and rs6000 set the builtin vector type's TYPE_NAME, >> and so one constructed via applying __attribute__((vector_size(16))) will >> never match. And it should. > > why? They only need to share the canonical type not the type node itself > (unless their name is the same, of course). Correct, that's what I meant. the canonical type equal function must match a just-consed vector with the already-hashed builtin. > Now -- that name comparing is somewhat odd. We hash type "variants" > with different names the same (so setting the name after inserting sth into > the hash is allowed, but it will overwrite the old entry so any unnamed uses > up to now get a type with a name...). I guess we'd be better off leaving > only unnamed types in the hash and building a type variant whenever we > add a name to such type. Right, the name matching surprised me, and my first attempt was to remove it. but that breaks wchar_t. wchar_t is the same representation as int (pedantically, some integral type), but is a distinct type (in C++ at least, I don't think C cares). The canonical type system records language-level distinctness, not representation distinctness. The difference between wchar_t and vector types is the only way to get a type the same as wchar_t is to use 'wchar_t' (so we have to start with a type at least as canonical as what we want). Whereas vector types are constructed via applying attributes to some underlying scalar type (so we have to go find the canonical type). It would therefore seem that this code in set_underlying_type (c-common.c) is wrong: if (DECL_IS_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE) { if (TYPE_NAME (TREE_TYPE (x)) == 0) TYPE_NAME (TREE_TYPE (x)) = x; } And this rs6000.c code is totally bogus: tdecl = add_builtin_type ("__vector unsigned int", unsigned_V4SI_type_node); TYPE_NAME (unsigned_V4SI_type_node) = tdecl; <-- this line (note that at this point, add_builtin_type has already set the TYPE_NAME via set_underlying type) For C++'s creation of the wchar_t node, we'd need to special case setting the node's name in some way? [char16_t and char32_t have the same distinctness property as wchar_t. I think that's the complete set] nathan
On Tue, Apr 4, 2017 at 1:31 PM, Nathan Sidwell <nathan@acm.org> wrote: > On 04/04/2017 04:27 AM, Richard Biener wrote: >> >> On Mon, Apr 3, 2017 at 9:03 PM, Nathan Sidwell <nathan@acm.org> wrote: > > >>> However, as you can see there's already a get-out for COMPLEX_TYPE, and I >>> think the same is needed for VECTOR_TYPE. >> >> >> Hmm, but that is essentially a hack given that build_complex_type does >> things >> it shouldn't (assign a name to the type). > > > Oh, didn't know that. > >>> Both set_underlying_type and rs6000 set the builtin vector type's >>> TYPE_NAME, >>> and so one constructed via applying __attribute__((vector_size(16))) will >>> never match. And it should. >> >> >> why? They only need to share the canonical type not the type node itself >> (unless their name is the same, of course). > > > Correct, that's what I meant. the canonical type equal function must match > a just-consed vector with the already-hashed builtin. > >> Now -- that name comparing is somewhat odd. We hash type "variants" >> with different names the same (so setting the name after inserting sth >> into >> the hash is allowed, but it will overwrite the old entry so any unnamed >> uses >> up to now get a type with a name...). I guess we'd be better off leaving >> only unnamed types in the hash and building a type variant whenever we >> add a name to such type. > > > Right, the name matching surprised me, and my first attempt was to remove > it. but that breaks wchar_t. wchar_t is the same representation as int > (pedantically, some integral type), but is a distinct type (in C++ at least, > I don't think C cares). The canonical type system records language-level > distinctness, not representation distinctness. > > The difference between wchar_t and vector types is the only way to get a > type the same as wchar_t is to use 'wchar_t' (so we have to start with a > type at least as canonical as what we want). Whereas vector types are > constructed via applying attributes to some underlying scalar type (so we > have to go find the canonical type). > > It would therefore seem that this code in set_underlying_type (c-common.c) > is wrong: > if (DECL_IS_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE) > { > if (TYPE_NAME (TREE_TYPE (x)) == 0) > TYPE_NAME (TREE_TYPE (x)) = x; > } This is quite a fragile area -- you also have to watch dwarf2out.c. Most of the TYPE_NAME "hacks" are to make debuginfo happy. > And this rs6000.c code is totally bogus: > tdecl = add_builtin_type ("__vector unsigned int", > unsigned_V4SI_type_node); > TYPE_NAME (unsigned_V4SI_type_node) = tdecl; <-- this line Yeah, not sure what that like is for -- add_builtin_type properly adds a name. Note it may be again to make debuginfo happy ;) tree add_builtin_type (const char *name, tree type) { tree id = get_identifier (name); tree decl = build_decl (BUILTINS_LOCATION, TYPE_DECL, id, type); return lang_hooks.decls.pushdecl (decl); } this seems to miss setting TYPE_NAME (type) = decl - oh, that may be what set_underlying_type does via the langhook. > (note that at this point, add_builtin_type has already set the TYPE_NAME via > set_underlying type) > > For C++'s creation of the wchar_t node, we'd need to special case setting > the node's name in some way? > > [char16_t and char32_t have the same distinctness property as wchar_t. I > think that's the complete set] At this point I'd rather restrict fiddling in this fragile area in rs6000.c only ;) Does removing the TYPE_NAME setting fix things? Richard. > nathan > > -- > Nathan Sidwell
Index: tree.c =================================================================== --- tree.c (revision 246647) +++ tree.c (working copy) @@ -7005,10 +7005,11 @@ type_cache_hasher::equal (type_hash *a, if (a->hash != b->hash || TREE_CODE (a->type) != TREE_CODE (b->type) || TREE_TYPE (a->type) != TREE_TYPE (b->type) - || !attribute_list_equal (TYPE_ATTRIBUTES (a->type), - TYPE_ATTRIBUTES (b->type)) || (TREE_CODE (a->type) != COMPLEX_TYPE - && TYPE_NAME (a->type) != TYPE_NAME (b->type))) + && TREE_CODE (a->type) != VECTOR_TYPE + && TYPE_NAME (a->type) != TYPE_NAME (b->type)) + || !attribute_list_equal (TYPE_ATTRIBUTES (a->type), + TYPE_ATTRIBUTES (b->type))) return 0; /* Be careful about comparing arrays before and after the element type