Re: Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node (was: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types)

[Richard Biener]

On Thu, Sep 29, 2016 at 4:48 PM, Thomas Schwinge
 wrote:
> Hi Richard!
>
> On Mon, 19 Sep 2016 13:25:01 +0200, Richard Biener 
>  wrote:
>> On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
>>  wrote:
>> > On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener 
>> >  wrote:
>> >> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
>> >>  wrote:
>> >> > --- 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)
>> >> > [...]
>> >> >else if (TREE_CODE (node) == RECORD_TYPE)
>
>> [looks to me we miss handling of vector type components alltogether,
>> maybe there are no global vector type trees ...]
>
> Looks like it, yes.  Would a patch like the following be reasonable,
> which explicitly lists/handles all expected tree codes, or is something
> like that not feasible?  (That's a subset of tree codes I gathered by a
> partial run of the GCC testsuite, and libgomp testsuite; not claiming
> this is complete.)

I think it would be a nice thing to have indeed.

So -- I'm inclined to approve this patch ;)

Thanks,
Richard.

> commit f28dd9618be8a26c6a75ee089f1755e4e0281106
> Author: Thomas Schwinge 
> Date:   Thu Sep 29 16:35:19 2016 +0200
>
> Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node
> ---
>  gcc/tree-streamer.c | 32 +---
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git gcc/tree-streamer.c gcc/tree-streamer.c
> index 6ada89a..8567a81 100644
> --- gcc/tree-streamer.c
> +++ gcc/tree-streamer.c
> @@ -303,17 +303,32 @@ record_common_node (struct streamer_tree_cache_d 
> *cache, tree node)
>   in the cache as hash value.  */
>streamer_tree_cache_append (cache, node, cache->nodes.length ());
>
> -  if (POINTER_TYPE_P (node)
> -  || TREE_CODE (node) == ARRAY_TYPE)
> -record_common_node (cache, TREE_TYPE (node));
> -  else if (TREE_CODE (node) == COMPLEX_TYPE)
> +  switch (TREE_CODE (node))
>  {
> +case ERROR_MARK:
> +case FIELD_DECL:
> +case FIXED_POINT_TYPE:
> +case IDENTIFIER_NODE:
> +case INTEGER_CST:
> +case INTEGER_TYPE:
> +case POINTER_BOUNDS_TYPE:
> +case REAL_TYPE:
> +case TREE_LIST:
> +case VOID_CST:
> +case VOID_TYPE:
> +  /* No recursion.  */
> +  break;
> +case POINTER_TYPE:
> +case REFERENCE_TYPE:
> +case ARRAY_TYPE:
> +  record_common_node (cache, TREE_TYPE (node));
> +  break;
> +case COMPLEX_TYPE:
>/* Verify that a complex type's component type (node_type) has been
>  handled already (and we thus don't need to recurse here).  */
>verify_common_node_recorded (cache, TREE_TYPE (node));
> -}
> -  else if (TREE_CODE (node) == RECORD_TYPE)
> -{
> +  break;
> +case RECORD_TYPE:
>/* The FIELD_DECLs of structures should be shared, so that every
>  COMPONENT_REF uses the same tree node when referencing a field.
>  Pointer equality between FIELD_DECLs is used by the alias
> @@ -322,6 +337,9 @@ record_common_node (struct streamer_tree_cache_d *cache, 
> tree node)
>  nonoverlapping_component_refs_of_decl_p).  */
>for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
> record_common_node (cache, f);
> +  break;
> +default:
> +  gcc_unreachable ();
>  }
>  }
>
>
>
> Grüße
>  Thomas

Re: Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node (was: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types)

[Richard Biener]

On Thu, Sep 29, 2016 at 4:48 PM, Thomas Schwinge
 wrote:
> Hi Richard!
>
> On Mon, 19 Sep 2016 13:25:01 +0200, Richard Biener 
>  wrote:
>> On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
>>  wrote:
>> > On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener 
>> >  wrote:
>> >> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
>> >>  wrote:
>> >> > --- 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)
>> >> > [...]
>> >> >else if (TREE_CODE (node) == RECORD_TYPE)
>
>> [looks to me we miss handling of vector type components alltogether,
>> maybe there are no global vector type trees ...]
>
> Looks like it, yes.  Would a patch like the following be reasonable,
> which explicitly lists/handles all expected tree codes, or is something
> like that not feasible?  (That's a subset of tree codes I gathered by a
> partial run of the GCC testsuite, and libgomp testsuite; not claiming
> this is complete.)

I think it would be a nice thing to have indeed.

So -- I'm inclined to approve this patch ;)

Thanks,
Richard.

> commit f28dd9618be8a26c6a75ee089f1755e4e0281106
> Author: Thomas Schwinge 
> Date:   Thu Sep 29 16:35:19 2016 +0200
>
> Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node
> ---
>  gcc/tree-streamer.c | 32 +---
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git gcc/tree-streamer.c gcc/tree-streamer.c
> index 6ada89a..8567a81 100644
> --- gcc/tree-streamer.c
> +++ gcc/tree-streamer.c
> @@ -303,17 +303,32 @@ record_common_node (struct streamer_tree_cache_d 
> *cache, tree node)
>   in the cache as hash value.  */
>streamer_tree_cache_append (cache, node, cache->nodes.length ());
>
> -  if (POINTER_TYPE_P (node)
> -  || TREE_CODE (node) == ARRAY_TYPE)
> -record_common_node (cache, TREE_TYPE (node));
> -  else if (TREE_CODE (node) == COMPLEX_TYPE)
> +  switch (TREE_CODE (node))
>  {
> +case ERROR_MARK:
> +case FIELD_DECL:
> +case FIXED_POINT_TYPE:
> +case IDENTIFIER_NODE:
> +case INTEGER_CST:
> +case INTEGER_TYPE:
> +case POINTER_BOUNDS_TYPE:
> +case REAL_TYPE:
> +case TREE_LIST:
> +case VOID_CST:
> +case VOID_TYPE:
> +  /* No recursion.  */
> +  break;
> +case POINTER_TYPE:
> +case REFERENCE_TYPE:
> +case ARRAY_TYPE:
> +  record_common_node (cache, TREE_TYPE (node));
> +  break;
> +case COMPLEX_TYPE:
>/* Verify that a complex type's component type (node_type) has been
>  handled already (and we thus don't need to recurse here).  */
>verify_common_node_recorded (cache, TREE_TYPE (node));
> -}
> -  else if (TREE_CODE (node) == RECORD_TYPE)
> -{
> +  break;
> +case RECORD_TYPE:
>/* The FIELD_DECLs of structures should be shared, so that every
>  COMPONENT_REF uses the same tree node when referencing a field.
>  Pointer equality between FIELD_DECLs is used by the alias
> @@ -322,6 +337,9 @@ record_common_node (struct streamer_tree_cache_d *cache, 
> tree node)
>  nonoverlapping_component_refs_of_decl_p).  */
>for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
> record_common_node (cache, f);
> +  break;
> +default:
> +  gcc_unreachable ();
>  }
>  }
>
>
>
> Grüße
>  Thomas

Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node (was: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types)

[Thomas Schwinge]

Hi Richard!

On Mon, 19 Sep 2016 13:25:01 +0200, Richard Biener  
wrote:
> On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
>  wrote:
> > On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener 
> >  wrote:
> >> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
> >>  wrote:
> >> > --- 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)
> >> > [...]
> >> >else if (TREE_CODE (node) == RECORD_TYPE)

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

Looks like it, yes.  Would a patch like the following be reasonable,
which explicitly lists/handles all expected tree codes, or is something
like that not feasible?  (That's a subset of tree codes I gathered by a
partial run of the GCC testsuite, and libgomp testsuite; not claiming
this is complete.)

commit f28dd9618be8a26c6a75ee089f1755e4e0281106
Author: Thomas Schwinge 
Date:   Thu Sep 29 16:35:19 2016 +0200

Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node
---
 gcc/tree-streamer.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git gcc/tree-streamer.c gcc/tree-streamer.c
index 6ada89a..8567a81 100644
--- gcc/tree-streamer.c
+++ gcc/tree-streamer.c
@@ -303,17 +303,32 @@ record_common_node (struct streamer_tree_cache_d *cache, 
tree node)
  in the cache as hash value.  */
   streamer_tree_cache_append (cache, node, cache->nodes.length ());
 
-  if (POINTER_TYPE_P (node)
-  || TREE_CODE (node) == ARRAY_TYPE)
-record_common_node (cache, TREE_TYPE (node));
-  else if (TREE_CODE (node) == COMPLEX_TYPE)
+  switch (TREE_CODE (node))
 {
+case ERROR_MARK:
+case FIELD_DECL:
+case FIXED_POINT_TYPE:
+case IDENTIFIER_NODE:
+case INTEGER_CST:
+case INTEGER_TYPE:
+case POINTER_BOUNDS_TYPE:
+case REAL_TYPE:
+case TREE_LIST:
+case VOID_CST:
+case VOID_TYPE:
+  /* No recursion.  */
+  break;
+case POINTER_TYPE:
+case REFERENCE_TYPE:
+case ARRAY_TYPE:
+  record_common_node (cache, TREE_TYPE (node));
+  break;
+case COMPLEX_TYPE:
   /* Verify that a complex type's component type (node_type) has been
 handled already (and we thus don't need to recurse here).  */
   verify_common_node_recorded (cache, TREE_TYPE (node));
-}
-  else if (TREE_CODE (node) == RECORD_TYPE)
-{
+  break;
+case RECORD_TYPE:
   /* The FIELD_DECLs of structures should be shared, so that every
 COMPONENT_REF uses the same tree node when referencing a field.
 Pointer equality between FIELD_DECLs is used by the alias
@@ -322,6 +337,9 @@ record_common_node (struct streamer_tree_cache_d *cache, 
tree node)
 nonoverlapping_component_refs_of_decl_p).  */
   for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
record_common_node (cache, f);
+  break;
+default:
+  gcc_unreachable ();
 }
 }
 


Grüße
 Thomas

Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node (was: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types)

[Thomas Schwinge]

Hi Richard!

On Mon, 19 Sep 2016 13:25:01 +0200, Richard Biener  
wrote:
> On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge
>  wrote:
> > On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener 
> >  wrote:
> >> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge
> >>  wrote:
> >> > --- 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)
> >> > [...]
> >> >else if (TREE_CODE (node) == RECORD_TYPE)

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

Looks like it, yes.  Would a patch like the following be reasonable,
which explicitly lists/handles all expected tree codes, or is something
like that not feasible?  (That's a subset of tree codes I gathered by a
partial run of the GCC testsuite, and libgomp testsuite; not claiming
this is complete.)

commit f28dd9618be8a26c6a75ee089f1755e4e0281106
Author: Thomas Schwinge 
Date:   Thu Sep 29 16:35:19 2016 +0200

Explicitly list all tree codes in gcc/tree-streamer.c:record_common_node
---
 gcc/tree-streamer.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git gcc/tree-streamer.c gcc/tree-streamer.c
index 6ada89a..8567a81 100644
--- gcc/tree-streamer.c
+++ gcc/tree-streamer.c
@@ -303,17 +303,32 @@ record_common_node (struct streamer_tree_cache_d *cache, 
tree node)
  in the cache as hash value.  */
   streamer_tree_cache_append (cache, node, cache->nodes.length ());
 
-  if (POINTER_TYPE_P (node)
-  || TREE_CODE (node) == ARRAY_TYPE)
-record_common_node (cache, TREE_TYPE (node));
-  else if (TREE_CODE (node) == COMPLEX_TYPE)
+  switch (TREE_CODE (node))
 {
+case ERROR_MARK:
+case FIELD_DECL:
+case FIXED_POINT_TYPE:
+case IDENTIFIER_NODE:
+case INTEGER_CST:
+case INTEGER_TYPE:
+case POINTER_BOUNDS_TYPE:
+case REAL_TYPE:
+case TREE_LIST:
+case VOID_CST:
+case VOID_TYPE:
+  /* No recursion.  */
+  break;
+case POINTER_TYPE:
+case REFERENCE_TYPE:
+case ARRAY_TYPE:
+  record_common_node (cache, TREE_TYPE (node));
+  break;
+case COMPLEX_TYPE:
   /* Verify that a complex type's component type (node_type) has been
 handled already (and we thus don't need to recurse here).  */
   verify_common_node_recorded (cache, TREE_TYPE (node));
-}
-  else if (TREE_CODE (node) == RECORD_TYPE)
-{
+  break;
+case RECORD_TYPE:
   /* The FIELD_DECLs of structures should be shared, so that every
 COMPONENT_REF uses the same tree node when referencing a field.
 Pointer equality between FIELD_DECLs is used by the alias
@@ -322,6 +337,9 @@ record_common_node (struct streamer_tree_cache_d *cache, 
tree node)
 nonoverlapping_component_refs_of_decl_p).  */
   for (tree f = TYPE_FIELDS (node); f; f = TREE_CHAIN (f))
record_common_node (cache, f);
+  break;
+default:
+  gcc_unreachable ();
 }
 }
 


Grüße
 Thomas