| Message ID | 20151210201116.GC5675@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/10/2015 01:11 PM, Jakub Jelinek wrote: > Hi! > > It seems some passes in between the combiner and ira aren't prepared to > update dominance info. It usually is not a problem, because already before > the combiner we call free_dominance_info. But we now have a new i?86 > stv pass that is injected after the combiner that computes dominators but > does not free them. > > So, to fix ICE on the following testcase, we can either do what the patch > does, or could conditionalize both the calculate_dominance_info and > free_dominance_info in the convert_scalars_to_vector function (stv pass) > on the dominance info not being computed (like other places in gcc do), > or we could stick free_dominance_info into all passes that break the > dominators just in case it would be computed (out_of_cfglayout is one > example). > > Bootstrapped/regtested on x86_64-linux and i686-linux, is this ok for trunk > (or some other variant is preferrable)? > > 2015-12-10 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/68730 > * config/i386/i386.c (convert_scalars_to_vector): Call > free_dominance_info at the end. > > * gcc.dg/pr68730.c: New test. Any pass that mucks up the dominator tree ought to be wiping it clean. It's obviously better if wiping the dominator tree is conditional on the pass actually making transformations to the CFG that invalidate the stored information. Similarly, any pass that needs the dominator tree ought to make sure it's around. Note this is cheap if the prior pass hasn't wiped the dominator tree. At least that's always been my understanding. So ISTM this patch is the right thing to do in and of itself, though it may not be complete as there may be passes that aren't following the rules noted above. jeff
On December 10, 2015 9:11:16 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >It seems some passes in between the combiner and ira aren't prepared to >update dominance info. It usually is not a problem, because already >before >the combiner we call free_dominance_info. But we now have a new i?86 >stv pass that is injected after the combiner that computes dominators >but >does not free them. > >So, to fix ICE on the following testcase, we can either do what the >patch >does, or could conditionalize both the calculate_dominance_info and >free_dominance_info in the convert_scalars_to_vector function (stv >pass) >on the dominance info not being computed (like other places in gcc do), >or we could stick free_dominance_info into all passes that break the >dominators just in case it would be computed (out_of_cfglayout is one >example). We rely on this everywhere else so that would be preferred. Richard. > >Bootstrapped/regtested on x86_64-linux and i686-linux, is this ok for >trunk >(or some other variant is preferrable)? > >2015-12-10 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/68730 > * config/i386/i386.c (convert_scalars_to_vector): Call > free_dominance_info at the end. > > * gcc.dg/pr68730.c: New test. > >--- gcc/config/i386/i386.c.jj 2015-12-09 14:39:02.000000000 +0100 >+++ gcc/config/i386/i386.c 2015-12-10 12:15:59.517609392 +0100 >@@ -3577,6 +3577,7 @@ convert_scalars_to_vector () > BITMAP_FREE (candidates); > bitmap_obstack_release (NULL); > df_process_deferred_rescans (); >+ free_dominance_info (CDI_DOMINATORS); > > /* Conversion means we may have 128bit register spills/fills > which require aligned stack. */ >--- gcc/testsuite/gcc.dg/pr68730.c.jj 2015-12-10 12:22:07.330365019 >+0100 >+++ gcc/testsuite/gcc.dg/pr68730.c 2015-12-10 12:24:03.908702426 +0100 >@@ -0,0 +1,51 @@ >+/* PR rtl-optimization/68730 */ >+/* { dg-do compile } */ >+/* { dg-options "-O3 -fno-if-conversion" } */ >+/* { dg-additional-options "-march=x86-64" { target { i?86-*-* >x86_64-*-* } } } */ >+ >+int b, d, e; >+unsigned long long c = 4100543410106915; >+ >+void >+foo (void) >+{ >+ short f, g = 4 % c; >+ int h = c; >+ if (h) >+ { >+ int i = ~c; >+ if (~c) >+ i = 25662; >+ f = g = i; >+ h = c - g + ~-f; >+ c = ~(c * h - f); >+ } >+ f = g; >+ unsigned long long k = g || c; >+ short l = c ^ g ^ k; >+ if (g > 25662 || c == 74074520320 || !(g < 2)) >+ { >+ k = c; >+ l = g; >+ c = ~((k && c) + ~l); >+ f = ~(f * (c ^ k) | l); >+ if (c > k) >+ __builtin_printf ("%d\n", f); >+ } >+ short m = -f; >+ unsigned long long n = c; >+ c = m * f | n % c; >+ if (n) >+ __builtin_printf ("%d\n", f); >+ while (f < -31807) >+ ; >+ c = ~(n | c) | f; >+ if (n < c) >+ __builtin_printf ("%lld\n", (long long) f); >+ for (; d;) >+ for (; e;) >+ for (;;) >+ ; >+ c = h; >+ c = l % c; >+} > > Jakub
--- gcc/config/i386/i386.c.jj 2015-12-09 14:39:02.000000000 +0100 +++ gcc/config/i386/i386.c 2015-12-10 12:15:59.517609392 +0100 @@ -3577,6 +3577,7 @@ convert_scalars_to_vector () BITMAP_FREE (candidates); bitmap_obstack_release (NULL); df_process_deferred_rescans (); + free_dominance_info (CDI_DOMINATORS); /* Conversion means we may have 128bit register spills/fills which require aligned stack. */ --- gcc/testsuite/gcc.dg/pr68730.c.jj 2015-12-10 12:22:07.330365019 +0100 +++ gcc/testsuite/gcc.dg/pr68730.c 2015-12-10 12:24:03.908702426 +0100 @@ -0,0 +1,51 @@ +/* PR rtl-optimization/68730 */ +/* { dg-do compile } */ +/* { dg-options "-O3 -fno-if-conversion" } */ +/* { dg-additional-options "-march=x86-64" { target { i?86-*-* x86_64-*-* } } } */ + +int b, d, e; +unsigned long long c = 4100543410106915; + +void +foo (void) +{ + short f, g = 4 % c; + int h = c; + if (h) + { + int i = ~c; + if (~c) + i = 25662; + f = g = i; + h = c - g + ~-f; + c = ~(c * h - f); + } + f = g; + unsigned long long k = g || c; + short l = c ^ g ^ k; + if (g > 25662 || c == 74074520320 || !(g < 2)) + { + k = c; + l = g; + c = ~((k && c) + ~l); + f = ~(f * (c ^ k) | l); + if (c > k) + __builtin_printf ("%d\n", f); + } + short m = -f; + unsigned long long n = c; + c = m * f | n % c; + if (n) + __builtin_printf ("%d\n", f); + while (f < -31807) + ; + c = ~(n | c) | f; + if (n < c) + __builtin_printf ("%lld\n", (long long) f); + for (; d;) + for (; e;) + for (;;) + ; + c = h; + c = l % c; +}