diff mbox

make find_taken_edge handle case with just default

Message ID ec513b23-24b4-250c-3ee8-2a0ee4b5beb4@vnet.ibm.com
State New
Headers show

Commit Message

Peter Bergner June 29, 2017, 1:55 p.m. UTC
On 6/29/17 4:03 AM, Richard Biener wrote:
> 
> This refactors things a bit to make CFG cleanup handle switches with
> just a default label.  If we make sure to cleanup the CFG after
> group_case_labels removes cases with just __builtin_unreachable ()
> inside then this fixes the ICE seen in PR81994 as well.
> 
> I wonder if find_taken_edge should generally handle successors
> with __builtin_unreachable () -- OTOH that would get rid of those
> too early I guess.

Should we offer an early out of group_case_labels_stmt() for the
fairly common case of new_size == old_size?  There's no reason to
execute the compress labels loop if we didn't combine any of the
labels.

Peter

Comments

Richard Biener June 29, 2017, 1:58 p.m. UTC | #1
On Thu, 29 Jun 2017, Peter Bergner wrote:

> On 6/29/17 4:03 AM, Richard Biener wrote:
> > 
> > This refactors things a bit to make CFG cleanup handle switches with
> > just a default label.  If we make sure to cleanup the CFG after
> > group_case_labels removes cases with just __builtin_unreachable ()
> > inside then this fixes the ICE seen in PR81994 as well.
> > 
> > I wonder if find_taken_edge should generally handle successors
> > with __builtin_unreachable () -- OTOH that would get rid of those
> > too early I guess.
> 
> Should we offer an early out of group_case_labels_stmt() for the
> fairly common case of new_size == old_size?  There's no reason to
> execute the compress labels loop if we didn't combine any of the
> labels.

We can also merge both loops, counting new_size upwards, storing
to label new_size if new_size != i ...

> Peter
> 
> 
> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c	(revision 249783)
> +++ gcc/tree-cfg.c	(working copy)
> @@ -1747,6 +1747,11 @@ group_case_labels_stmt (gswitch *stmt)
>  	}
>      }
> 
> +  gcc_assert (new_size <= old_size);
> +
> +  if (new_size == old_size)
> +    return false;
> +
>    /* Compress the case labels in the label vector, and adjust the
>       length of the vector.  */
>    for (i = 0, j = 0; i < new_size; i++)
> @@ -1757,9 +1762,8 @@ group_case_labels_stmt (gswitch *stmt)
>  			       gimple_switch_label (stmt, j++));
>      }
> 
> -  gcc_assert (new_size <= old_size);
>    gimple_switch_set_num_labels (stmt, new_size);
> -  return new_size < old_size;
> +  return true;
>  }
> 
>  /* Look for blocks ending in a multiway branch (a GIMPLE_SWITCH),
> 
>
Peter Bergner June 29, 2017, 2:11 p.m. UTC | #2
On 6/29/17 8:58 AM, Richard Biener wrote:
> On Thu, 29 Jun 2017, Peter Bergner wrote:
>> Should we offer an early out of group_case_labels_stmt() for the
>> fairly common case of new_size == old_size?  There's no reason to
>> execute the compress labels loop if we didn't combine any of the
>> labels.
> 
> We can also merge both loops, counting new_size upwards, storing
> to label new_size if new_size != i ...

Yeah.  I can implement that if you like...unless you want to...
or already have. :-)

Peter
Richard Biener June 29, 2017, 2:22 p.m. UTC | #3
On Thu, 29 Jun 2017, Peter Bergner wrote:

> On 6/29/17 8:58 AM, Richard Biener wrote:
> > On Thu, 29 Jun 2017, Peter Bergner wrote:
> >> Should we offer an early out of group_case_labels_stmt() for the
> >> fairly common case of new_size == old_size?  There's no reason to
> >> execute the compress labels loop if we didn't combine any of the
> >> labels.
> > 
> > We can also merge both loops, counting new_size upwards, storing
> > to label new_size if new_size != i ...
> 
> Yeah.  I can implement that if you like...unless you want to...
> or already have. :-)

Go ahead!

Richard.
diff mbox

Patch

Index: gcc/tree-cfg.c
===================================================================
--- gcc/tree-cfg.c	(revision 249783)
+++ gcc/tree-cfg.c	(working copy)
@@ -1747,6 +1747,11 @@  group_case_labels_stmt (gswitch *stmt)
 	}
     }

+  gcc_assert (new_size <= old_size);
+
+  if (new_size == old_size)
+    return false;
+
   /* Compress the case labels in the label vector, and adjust the
      length of the vector.  */
   for (i = 0, j = 0; i < new_size; i++)
@@ -1757,9 +1762,8 @@  group_case_labels_stmt (gswitch *stmt)
 			       gimple_switch_label (stmt, j++));
     }

-  gcc_assert (new_size <= old_size);
   gimple_switch_set_num_labels (stmt, new_size);
-  return new_size < old_size;
+  return true;
 }

 /* Look for blocks ending in a multiway branch (a GIMPLE_SWITCH),