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

[Thomas Schwinge <thomas@codesourcery.com>]

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?

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