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 <ja...@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 <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)
