Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)

[Richard Biener <rguenther at suse dot de>]

On Fri, 9 Jan 2015, Richard Biener wrote:

> On Fri, 9 Jan 2015, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > The following testcase is miscompiled on s390x.  The problem is that there
> > is massive cross-jumping going on, and after that post_order_compute
> > decides to call tidy_fallthru_edges, including on an edge from a bb ending
> > with a table jump to a bb with now a single successor where all the
> > jump_table_data entries point to.  rtl_tidy_fallthru_edge happily removes
> > the tablejump and anything in between the current bb and next bb (i.e.
> > jump_table_data + its code_label + barrier if any), but doesn't care about
> > any possible uses of the code_label (on the testcase e.g. the label
> > reference is hoisted before the loop).
> > Now, if I try some artificial testcase like:
> > int a;
> > #define A(n) case n: a++; break;
> > #define B(n) A(n##0) A(n##1) A(n##2) A(n##3) A(n##4) A(n##5) A(n##6) A(n##7) A(n##8) A(n##9)
> > #define C(n) B(n##0) B(n##1) B(n##2) B(n##3) B(n##4) B(n##5) B(n##6) B(n##7) B(n##8) B(n##9)
> > 
> > void
> > foo (int x)
> > {
> >   switch (x)
> >     {
> >     C(1)
> >     }
> > }
> > say on x86_64, tidy_fallthru_edges isn't called at all (it would be only if
> > there are unrelated unreachable blocks in the function at the same time),
> > so I think spending time on trying to handle tablejump_p right in
> > rtl_tidy_fallthru_edge is wasteful, my preference (for both 4.9 and trunk)
> > would be as the patch below just not handle tablejump_p in that function,
> > and for trunk perhaps try to handle it elsewhere where it will be optimized
> > even for the above testcase (somewhere in cfgcleanup?).
> 
> I wonder why post_order_compute calls tidy_fallthru_edges at all - won't
> that break the just computed postorder?
> 
> Other than that, why doesn't can't the issue show up with non-table-jumps?
> 
> What does it take to preserve (all) the labels?

That is, just remove q, not everything between q and BB_HEAD (c)?

> Richard.
> 
> > Also, eventually it would be really nice if tree-ssa-tail-merge.c could
> > handle this already at the GIMPLE level.
> > 
> > Thoughts on this?
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux on the trunk and on
> > {x86_64,i686,ppc64,ppc64le,s390,s390x,armv7hl}-linux on 4.9 branch.
> > 
> > 2015-01-09  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR rtl-optimization/64536
> > 	* cfgrtl.c (rtl_tidy_fallthru_edge): Don't remove tablejumps.
> > 
> > 	* gcc.dg/pr64536.c: New test.
> > 
> > --- gcc/cfgrtl.c.jj	2015-01-05 13:07:12.000000000 +0100
> > +++ gcc/cfgrtl.c	2015-01-08 17:03:18.511218340 +0100
> > @@ -1782,10 +1782,14 @@ rtl_tidy_fallthru_edge (edge e)
> >      if (INSN_P (q))
> >        return;
> >  
> > +  q = BB_END (b);
> > +  /* Don't remove table jumps here.  */
> > +  if (tablejump_p (q, NULL, NULL))
> > +    return;
> > +
> >    /* Remove what will soon cease being the jump insn from the source block.
> >       If block B consisted only of this single jump, turn it into a deleted
> >       note.  */
> > -  q = BB_END (b);
> >    if (JUMP_P (q)
> >        && onlyjump_p (q)
> >        && (any_uncondjump_p (q)
> > --- gcc/testsuite/gcc.dg/pr64536.c.jj	2015-01-08 17:13:32.218929003 +0100
> > +++ gcc/testsuite/gcc.dg/pr64536.c	2015-01-08 17:28:56.758428958 +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
> > 
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)