[PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6]))

[Richard Biener]

On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
> Hi!
>
> On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener <richard.guenther@gmail.com> wrote:
>> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
>> <thomas@codesourcery.com> wrote:
>> > --- gcc/tree-core.h
>> > +++ gcc/tree-core.h
>> > @@ -553,20 +553,6 @@ enum tree_index {
>> >    TI_BOOLEAN_FALSE,
>> >    TI_BOOLEAN_TRUE,
>> >
>> > -  TI_COMPLEX_INTEGER_TYPE,
>> > -[...]
>> > -  TI_COMPLEX_FLOAT128X_TYPE,
>> > -
>> >    TI_FLOAT_TYPE,
>> >    TI_DOUBLE_TYPE,
>> >    TI_LONG_DOUBLE_TYPE,
>> > @@ -596,6 +582,23 @@ enum tree_index {
>> >                              - TI_FLOATN_NX_TYPE_FIRST          \
>> >                              + 1)
>> >
>> > +  /* Put the complex types after their component types, so that in (sequential)
>> > +     tree streaming we can assert that their component types have already been
>> > +     handled (see tree-streamer.c:record_common_node).  */
>> > +  TI_COMPLEX_INTEGER_TYPE,
>> > +  TI_COMPLEX_FLOAT_TYPE,
>> > +  TI_COMPLEX_DOUBLE_TYPE,
>> > +  TI_COMPLEX_LONG_DOUBLE_TYPE,
>> > +
>> > +  TI_COMPLEX_FLOAT16_TYPE,
>> > +  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
>> > +  TI_COMPLEX_FLOAT32_TYPE,
>> > +  TI_COMPLEX_FLOAT64_TYPE,
>> > +  TI_COMPLEX_FLOAT128_TYPE,
>> > +  TI_COMPLEX_FLOAT32X_TYPE,
>> > +  TI_COMPLEX_FLOAT64X_TYPE,
>> > +  TI_COMPLEX_FLOAT128X_TYPE,
>> > +
>> >    TI_FLOAT_PTR_TYPE,
>> >    TI_DOUBLE_PTR_TYPE,
>> >    TI_LONG_DOUBLE_PTR_TYPE,
>>
>> If the above change alone fixes your issues then it is fine to commit.
>
> That alone won't fix the problem, because we'd still have the recursion
> in gcc/tree-streamer.c:record_common_node done differently for x86_64
> target and nvptx offload target.

Doh - obviously.

>> > --- gcc/tree-streamer.c
>> > +++ gcc/tree-streamer.c
>> > @@ -278,9 +278,23 @@ record_common_node (struct streamer_tree_cache_d *cache, tree node)
>> >    streamer_tree_cache_append (cache, node, cache->nodes.length ());
>> >
>> >    if (POINTER_TYPE_P (node)
>> > -      || TREE_CODE (node) == COMPLEX_TYPE
>> >        || TREE_CODE (node) == ARRAY_TYPE)
>> >      record_common_node (cache, TREE_TYPE (node));
>> > +  else if (TREE_CODE (node) == COMPLEX_TYPE)
>> > +    {
>> > +      /* Assert that complex types' component types have already been handled
>> > +        (and we thus don't need to recurse here).  See PR lto/77458.  */
>> > +      if (cache->node_map)
>> > +       gcc_assert (streamer_tree_cache_lookup (cache, TREE_TYPE (node), NULL));
>> > +      else
>> > +       {
>> > +         gcc_assert (cache->nodes.exists ());
>> > +         bool found = false;
>> > +         for (unsigned i = 0; !found && i < cache->nodes.length (); ++i)
>> > +           found = true;
>>
>> hmm, this doesn't actually test anything? ;)
>
> ;-) Haha, hooray for patch review!
>
>> > +         gcc_assert (found);
>> > +       }
>> > +    }
>> >    else if (TREE_CODE (node) == RECORD_TYPE)
>> >      {
>> >        /* The FIELD_DECLs of structures should be shared, so that every
>
>> So I very much like to go forward with this kind of change as well
>
> OK, good.  So, in plain text, we'll make it a requirement that:
> integer_types trees must only refer to earlier integer_types trees;
> sizetype_tab trees must only refer to integer_types trees, and earlier
> sizetype_tab trees; and global_trees must only refer to integer_types
> trees, sizetype_tab trees, and earlier global_trees.

Yeah, though I'd put sizetypes first.

>> (the assert code
>> should go to a separate helper function).
>
> Should this checking be done only in
> gcc/tree-streamer.c:record_common_node, or should generally

Yes.

> gcc/tree-streamer.c:streamer_tree_cache_append check/assert that such
> recursive trees are already present in the cache?  And generally do that,
> or "if (flag_checking)" only?

I think we should restrict it to flag_checking only because in general violating
it is harmless plus we never know what happens on all targets/frontend/flag(!)
combinations.

>
>> Did you try it on more than just
>> the complex type case?
>
> Not yet, but now that you have approved the general concept, I'll look
> into that.
>
> Here's the current patch with the assertion condition fixed, but still
> for complex types only.  OK for trunk already?

Ok with the checking blob moved to a helper function,

  bool common_node_recorded_p (cache, node)

and its body guarded with if (flag_checking).

[looks to me we miss handling of vector type components alltogether,
maybe there are no global vector type trees ...]

Thanks,
Richard.

> --- gcc/tree-core.h
> +++ gcc/tree-core.h
> @@ -553,20 +553,6 @@ enum tree_index {
>    TI_BOOLEAN_FALSE,
>    TI_BOOLEAN_TRUE,
>
> -  TI_COMPLEX_INTEGER_TYPE,
> -  TI_COMPLEX_FLOAT_TYPE,
> -  TI_COMPLEX_DOUBLE_TYPE,
> -  TI_COMPLEX_LONG_DOUBLE_TYPE,
> -
> -  TI_COMPLEX_FLOAT16_TYPE,
> -  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
> -  TI_COMPLEX_FLOAT32_TYPE,
> -  TI_COMPLEX_FLOAT64_TYPE,
> -  TI_COMPLEX_FLOAT128_TYPE,
> -  TI_COMPLEX_FLOAT32X_TYPE,
> -  TI_COMPLEX_FLOAT64X_TYPE,
> -  TI_COMPLEX_FLOAT128X_TYPE,
> -
>    TI_FLOAT_TYPE,
>    TI_DOUBLE_TYPE,
>    TI_LONG_DOUBLE_TYPE,
> @@ -596,6 +582,23 @@ enum tree_index {
>                              - TI_FLOATN_NX_TYPE_FIRST          \
>                              + 1)
>
> +  /* Put the complex types after their component types, so that in (sequential)
> +     tree streaming we can assert that their component types have already been
> +     handled (see tree-streamer.c:record_common_node).  */
> +  TI_COMPLEX_INTEGER_TYPE,
> +  TI_COMPLEX_FLOAT_TYPE,
> +  TI_COMPLEX_DOUBLE_TYPE,
> +  TI_COMPLEX_LONG_DOUBLE_TYPE,
> +
> +  TI_COMPLEX_FLOAT16_TYPE,
> +  TI_COMPLEX_FLOATN_NX_TYPE_FIRST = TI_COMPLEX_FLOAT16_TYPE,
> +  TI_COMPLEX_FLOAT32_TYPE,
> +  TI_COMPLEX_FLOAT64_TYPE,
> +  TI_COMPLEX_FLOAT128_TYPE,
> +  TI_COMPLEX_FLOAT32X_TYPE,
> +  TI_COMPLEX_FLOAT64X_TYPE,
> +  TI_COMPLEX_FLOAT128X_TYPE,
> +
>    TI_FLOAT_PTR_TYPE,
>    TI_DOUBLE_PTR_TYPE,
>    TI_LONG_DOUBLE_PTR_TYPE,
> --- gcc/tree-streamer.c
> +++ gcc/tree-streamer.c
> @@ -278,9 +278,26 @@ record_common_node (struct streamer_tree_cache_d *cache, tree node)
>    streamer_tree_cache_append (cache, node, cache->nodes.length ());
>
>    if (POINTER_TYPE_P (node)
> -      || TREE_CODE (node) == COMPLEX_TYPE
>        || TREE_CODE (node) == ARRAY_TYPE)
>      record_common_node (cache, TREE_TYPE (node));
> +  else if (TREE_CODE (node) == COMPLEX_TYPE)
> +    {
> +      /* Assert that a complex type's component type (node_type) has been
> +        handled already (and we thus don't need to recurse here).  See PR
> +        lto/77458.  */
> +      tree node_type = TREE_TYPE (node);
> +      if (cache->node_map)
> +       gcc_assert (streamer_tree_cache_lookup (cache, node_type, NULL));
> +      else
> +       {
> +         gcc_assert (cache->nodes.exists ());
> +         bool found = false;
> +         for (unsigned i = 0; !found && i < cache->nodes.length (); ++i)
> +           if (cache->nodes[i] == node_type)
> +             found = true;
> +         gcc_assert (found);
> +       }
> +    }
>    else if (TREE_CODE (node) == RECORD_TYPE)
>      {
>        /* The FIELD_DECLs of structures should be shared, so that every
>
>
> Grüße
>  Thomas