[PR 79905] ICE with vector_type

[Richard Biener]

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