[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]))

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.

> > --- 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.

> (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
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?

> 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?



Grüße
 Thomas

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