public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
@ 2015-01-09  9:27 Jakub Jelinek
  2015-01-09  9:44 ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2015-01-09  9:27 UTC (permalink / raw)
  To: Richard Biener, Eric Botcazou, Jeff Law; +Cc: gcc-patches

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?).
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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09  9:27 [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536) Jakub Jelinek
@ 2015-01-09  9:44 ` Richard Biener
  2015-01-09  9:50   ` Richard Biener
  2015-01-09  9:57   ` Jakub Jelinek
  0 siblings, 2 replies; 11+ messages in thread
From: Richard Biener @ 2015-01-09  9:44 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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?

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)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09  9:44 ` Richard Biener
@ 2015-01-09  9:50   ` Richard Biener
  2015-01-09  9:57   ` Jakub Jelinek
  1 sibling, 0 replies; 11+ messages in thread
From: Richard Biener @ 2015-01-09  9:50 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09  9:44 ` Richard Biener
  2015-01-09  9:50   ` Richard Biener
@ 2015-01-09  9:57   ` Jakub Jelinek
  2015-01-09 10:26     ` Richard Biener
  1 sibling, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2015-01-09  9:57 UTC (permalink / raw)
  To: Richard Biener; +Cc: Eric Botcazou, Jeff Law, gcc-patches

On Fri, Jan 09, 2015 at 10:36:09AM +0100, Richard Biener wrote:
> I wonder why post_order_compute calls tidy_fallthru_edges at all - won't
> that break the just computed postorder?

Dunno, but I think it shouldn't break anything, the function doesn't remove
any blocks, just in the typical case of an unconditional jump to the next bb
or conditional jump to the next bb (if only successor) removes the jump and
makes the edge EDGE_FALLTHRU.

> Other than that, why doesn't can't the issue show up with non-table-jumps?

I think tablejumps are the only case where (at least during jump2)
code_labels live in between the basic blocks, not inside of them.

> What does it take to preserve (all) the labels?

Then we'd need to remove all the instructions in between the two basic
blocks (as we currently do), but move any code_labels from there first to
the start of the next basic block.  Probably better just call tablejump_p
with non-NULL args and move precisely that code_label that it sets.

But, as I said, we'd still not optimize it if tidy_fallthru_edges is not
called, so we'd need to do it at another place too.

	Jakub

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09  9:57   ` Jakub Jelinek
@ 2015-01-09 10:26     ` Richard Biener
  2015-01-09 11:04       ` Jakub Jelinek
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2015-01-09 10:26 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

On Fri, 9 Jan 2015, Jakub Jelinek wrote:

> On Fri, Jan 09, 2015 at 10:36:09AM +0100, Richard Biener wrote:
> > I wonder why post_order_compute calls tidy_fallthru_edges at all - won't
> > that break the just computed postorder?
> 
> Dunno, but I think it shouldn't break anything, the function doesn't remove
> any blocks, just in the typical case of an unconditional jump to the next bb
> or conditional jump to the next bb (if only successor) removes the jump and
> makes the edge EDGE_FALLTHRU.
> 
> > Other than that, why doesn't can't the issue show up with non-table-jumps?
> 
> I think tablejumps are the only case where (at least during jump2)
> code_labels live in between the basic blocks, not inside of them.
> 
> > What does it take to preserve (all) the labels?
> 
> Then we'd need to remove all the instructions in between the two basic
> blocks (as we currently do), but move any code_labels from there first to
> the start of the next basic block.  Probably better just call tablejump_p
> with non-NULL args and move precisely that code_label that it sets.
> 
> But, as I said, we'd still not optimize it if tidy_fallthru_edges is not
> called, so we'd need to do it at another place too.

Ok, I see.  I still wonder why we call tidy_fallthru_edges from
postorder_compute.  If we delete unreachable blocks that means
we at most remove incoming edges to a block - that should never
change any other edges fallthru status...?

Does just removing that call there work and make the bug latent again?

Richard.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 10:26     ` Richard Biener
@ 2015-01-09 11:04       ` Jakub Jelinek
  2015-01-09 11:07         ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2015-01-09 11:04 UTC (permalink / raw)
  To: Richard Biener; +Cc: Eric Botcazou, Jeff Law, gcc-patches

On Fri, Jan 09, 2015 at 11:15:14AM +0100, Richard Biener wrote:
> On Fri, 9 Jan 2015, Jakub Jelinek wrote:
> 
> > On Fri, Jan 09, 2015 at 10:36:09AM +0100, Richard Biener wrote:
> > > I wonder why post_order_compute calls tidy_fallthru_edges at all - won't
> > > that break the just computed postorder?
> > 
> > Dunno, but I think it shouldn't break anything, the function doesn't remove
> > any blocks, just in the typical case of an unconditional jump to the next bb
> > or conditional jump to the next bb (if only successor) removes the jump and
> > makes the edge EDGE_FALLTHRU.
> > 
> > > Other than that, why doesn't can't the issue show up with non-table-jumps?
> > 
> > I think tablejumps are the only case where (at least during jump2)
> > code_labels live in between the basic blocks, not inside of them.
> > 
> > > What does it take to preserve (all) the labels?
> > 
> > Then we'd need to remove all the instructions in between the two basic
> > blocks (as we currently do), but move any code_labels from there first to
> > the start of the next basic block.  Probably better just call tablejump_p
> > with non-NULL args and move precisely that code_label that it sets.
> > 
> > But, as I said, we'd still not optimize it if tidy_fallthru_edges is not
> > called, so we'd need to do it at another place too.
> 
> Ok, I see.  I still wonder why we call tidy_fallthru_edges from
> postorder_compute.  If we delete unreachable blocks that means
> we at most remove incoming edges to a block - that should never
> change any other edges fallthru status...?

The call has been added by
http://gcc.gnu.org/ml/gcc-patches/2006-08/msg00095.html
and is only done if post_order_compute is called with the special flag,
supposedly that replaced explicit delete_unreachable_blocks or similar.
And, if you remove unreachable blocks, if they are in between some bb
and its single successor, then indeed that is something that should be
tidied, as we don't have to jump around nothing.

If you want, I can try instead of disabling it for tablejumps
just move the label.

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.

	Jakub

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 11:04       ` Jakub Jelinek
@ 2015-01-09 11:07         ` Richard Biener
  2015-01-09 14:10           ` Jakub Jelinek
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2015-01-09 11:07 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

On Fri, 9 Jan 2015, Jakub Jelinek wrote:

> On Fri, Jan 09, 2015 at 11:15:14AM +0100, Richard Biener wrote:
> > On Fri, 9 Jan 2015, Jakub Jelinek wrote:
> > 
> > > On Fri, Jan 09, 2015 at 10:36:09AM +0100, Richard Biener wrote:
> > > > I wonder why post_order_compute calls tidy_fallthru_edges at all - won't
> > > > that break the just computed postorder?
> > > 
> > > Dunno, but I think it shouldn't break anything, the function doesn't remove
> > > any blocks, just in the typical case of an unconditional jump to the next bb
> > > or conditional jump to the next bb (if only successor) removes the jump and
> > > makes the edge EDGE_FALLTHRU.
> > > 
> > > > Other than that, why doesn't can't the issue show up with non-table-jumps?
> > > 
> > > I think tablejumps are the only case where (at least during jump2)
> > > code_labels live in between the basic blocks, not inside of them.
> > > 
> > > > What does it take to preserve (all) the labels?
> > > 
> > > Then we'd need to remove all the instructions in between the two basic
> > > blocks (as we currently do), but move any code_labels from there first to
> > > the start of the next basic block.  Probably better just call tablejump_p
> > > with non-NULL args and move precisely that code_label that it sets.
> > > 
> > > But, as I said, we'd still not optimize it if tidy_fallthru_edges is not
> > > called, so we'd need to do it at another place too.
> > 
> > Ok, I see.  I still wonder why we call tidy_fallthru_edges from
> > postorder_compute.  If we delete unreachable blocks that means
> > we at most remove incoming edges to a block - that should never
> > change any other edges fallthru status...?
> 
> The call has been added by
> http://gcc.gnu.org/ml/gcc-patches/2006-08/msg00095.html
> and is only done if post_order_compute is called with the special flag,
> supposedly that replaced explicit delete_unreachable_blocks or similar.
> And, if you remove unreachable blocks, if they are in between some bb
> and its single successor, then indeed that is something that should be
> tidied, as we don't have to jump around nothing.

Ah, indeed.

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

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

Richard.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 11:07         ` Richard Biener
@ 2015-01-09 14:10           ` Jakub Jelinek
  2015-01-09 14:21             ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2015-01-09 14:10 UTC (permalink / raw)
  To: Richard Biener; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 14:10           ` Jakub Jelinek
@ 2015-01-09 14:21             ` Richard Biener
  2015-01-09 14:40               ` Jakub Jelinek
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Biener @ 2015-01-09 14:21 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 14:21             ` Richard Biener
@ 2015-01-09 14:40               ` Jakub Jelinek
  2015-01-12  9:18                 ` Richard Biener
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Jelinek @ 2015-01-09 14:40 UTC (permalink / raw)
  To: Richard Biener; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536)
  2015-01-09 14:40               ` Jakub Jelinek
@ 2015-01-12  9:18                 ` Richard Biener
  0 siblings, 0 replies; 11+ messages in thread
From: Richard Biener @ 2015-01-12  9:18 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Eric Botcazou, Jeff Law, gcc-patches

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-01-12  9:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-09  9:27 [PATCH] Fix undefined label problem after crossjumping (PR rtl-optimization/64536) Jakub Jelinek
2015-01-09  9:44 ` Richard Biener
2015-01-09  9:50   ` Richard Biener
2015-01-09  9:57   ` Jakub Jelinek
2015-01-09 10:26     ` Richard Biener
2015-01-09 11:04       ` Jakub Jelinek
2015-01-09 11:07         ` Richard Biener
2015-01-09 14:10           ` Jakub Jelinek
2015-01-09 14:21             ` Richard Biener
2015-01-09 14:40               ` Jakub Jelinek
2015-01-12  9:18                 ` Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).