Fix undefined label problem after crossjumping (PR rtl-optimization/64536)

On Fri, Jan 09, 2015 at 11:59:44AM +0100, Richard Biener wrote:
> > If you want, I can try instead of disabling it for tablejumps
> > just move the label.
> 
> Yeah, I'd prefer that - it can't be too difficult, no?

So like this (tested just on the testcase, fully bootstrap/regtest
will follow)?

> > Still, I think we should be able to optimize it somewhere else too
> > (we can remove the tablejumps not just if all jump_table_data entries
> > point to next_bb, but even when they point to some completely different bb,
> > as long as it is a single_succ_p).  And ideally also optimize it at GIMPLE,
> > but guess that is GCC 6 material.
> 
> cfgcleanup material, similar for GIMPLE I guess.

You mean that cfgcleanup changes are GCC 6 material too?

2015-01-09  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/64536
	* cfgrtl.c (rtl_tidy_fallthru_edge): Handle removal of degenerate
	tablejumps.

	* gcc.dg/pr64536.c: New test.



	Jakub

On Fri, 9 Jan 2015, Jakub Jelinek wrote:

> On Fri, Jan 09, 2015 at 11:59:44AM +0100, Richard Biener wrote:
> > > If you want, I can try instead of disabling it for tablejumps
> > > just move the label.
> > 
> > Yeah, I'd prefer that - it can't be too difficult, no?
> 
> So like this (tested just on the testcase, fully bootstrap/regtest
> will follow)?

Yeah.

> > > Still, I think we should be able to optimize it somewhere else too 
> > > (we can remove the tablejumps not just if all jump_table_data 
> > > entries point to next_bb, but even when they point to some 
> > > completely different bb, as long as it is a single_succ_p).  And 
> > > ideally also optimize it at GIMPLE, but guess that is GCC 6 
> > > material.
> > 
> > cfgcleanup material, similar for GIMPLE I guess.
> 
> You mean that cfgcleanup changes are GCC 6 material too?

Well, you have until the end of next week ;)  For GIMPLE this is
a switch with all cases going to the same basic-block, right?
I think we optimize that in cleanup_control_expr_graph via the
single_succ_p case?

Thanks,
Richard.

> 2015-01-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/64536
> 	* cfgrtl.c (rtl_tidy_fallthru_edge): Handle removal of degenerate
> 	tablejumps.
> 
> 	* gcc.dg/pr64536.c: New test.
> 
> --- gcc/cfgrtl.c.jj	2015-01-08 18:10:23.616598916 +0100
> +++ gcc/cfgrtl.c	2015-01-09 14:47:26.637855477 +0100
> @@ -1791,6 +1791,24 @@ rtl_tidy_fallthru_edge (edge e)
>        && (any_uncondjump_p (q)
>  	  || single_succ_p (b)))
>      {
> +      rtx label;
> +      rtx_jump_table_data *table;
> +
> +      if (tablejump_p (q, &label, &table))
> +	{
> +	  /* The label is likely mentioned in some instruction before
> +	     the tablejump and might not be DCEd, so turn it into
> +	     a note instead and move before the tablejump that is going to
> +	     be deleted.  */
> +	  const char *name = LABEL_NAME (label);
> +	  PUT_CODE (label, NOTE);
> +	  NOTE_KIND (label) = NOTE_INSN_DELETED_LABEL;
> +	  NOTE_DELETED_LABEL_NAME (label) = name;
> +	  rtx_insn *lab = safe_as_a <rtx_insn *> (label);
> +	  reorder_insns (lab, lab, PREV_INSN (q));
> +	  delete_insn (table);
> +	}
> +
>  #ifdef HAVE_cc0
>        /* If this was a conditional jump, we need to also delete
>  	 the insn that set cc0.  */
> --- gcc/testsuite/gcc.dg/pr64536.c.jj	2015-01-09 13:55:53.035267213 +0100
> +++ gcc/testsuite/gcc.dg/pr64536.c	2015-01-09 13:55:53.035267213 +0100
> @@ -0,0 +1,67 @@
> +/* PR rtl-optimization/64536 */
> +/* { dg-do link } */
> +/* { dg-options "-O2" } */
> +/* { dg-additional-options "-fPIC" { target fpic } } */
> +
> +struct S { long q; } *h;
> +long a, b, g, j, k, *c, *d, *e, *f, *i;
> +long *baz (void)
> +{
> +  asm volatile ("" : : : "memory");
> +  return e;
> +}
> +
> +void
> +bar (int x)
> +{
> +  int y;
> +  for (y = 0; y < x; y++)
> +    {
> +      switch (b)
> +	{
> +	case 0:
> +	case 2:
> +	  a++;
> +	  break;
> +	case 3:
> +	  a++;
> +	  break;
> +	case 1:
> +	  a++;
> +	}
> +      if (d)
> +	{
> +	  f = baz ();
> +	  g = k++;
> +	  if (&h->q)
> +	    {
> +	      j = *f;
> +	      h->q = *f;
> +	    }
> +	  else
> +	    i = (long *) (h->q = *f);
> +	  *c++ = (long) f;
> +	  e += 6;
> +	}
> +      else
> +	{
> +	  f = baz ();
> +	  g = k++;
> +	  if (&h->q)
> +	    {
> +	      j = *f;
> +	      h->q = *f;
> +	    }
> +	  else
> +	    i = (long *) (h->q = *f);
> +	  *c++ = (long) f;
> +	  e += 6;
> +	}
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> 
> 
> 	Jakub
> 
>

On Fri, Jan 09, 2015 at 03:10:16PM +0100, Richard Biener wrote:
> Well, you have until the end of next week ;)  For GIMPLE this is
> a switch with all cases going to the same basic-block, right?
> I think we optimize that in cleanup_control_expr_graph via the
> single_succ_p case?

No, it is a switch with cases that all look like:
  _1 = a; // load
  _2 = _1 + 1;
  a = _2; // store
So, either if tree-ssa-tail-merge could be tought about loads/stores,
or some other pass would be able to hoist the loads before the switch and
sink the store after the switch, because every switch case does that.

	Jakub

On Fri, 9 Jan 2015, Jakub Jelinek wrote:

> On Fri, Jan 09, 2015 at 03:10:16PM +0100, Richard Biener wrote:
> > Well, you have until the end of next week ;)  For GIMPLE this is
> > a switch with all cases going to the same basic-block, right?
> > I think we optimize that in cleanup_control_expr_graph via the
> > single_succ_p case?
> 
> No, it is a switch with cases that all look like:
>   _1 = a; // load
>   _2 = _1 + 1;
>   a = _2; // store
> So, either if tree-ssa-tail-merge could be tought about loads/stores,
> or some other pass would be able to hoist the loads before the switch and
> sink the store after the switch, because every switch case does that.

Ah, ok.  Indeed code-hoisting on GIMPLE wasn't finished (there is a
very old PR with patches still), and sinking has the same issue
in that it only exploits partial dead code elimination opportunities.

I think that tail-merging already handles some of these cases, just
maybe not the one with more than two PHI args or switches.

Richard.