[PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[Jakub Jelinek]

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.

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

Re: [PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[Jeff Law]

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

Re: [PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[Richard Biener]

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

Re: [PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[Jakub Jelinek]

On Fri, Dec 11, 2015 at 07:30:59AM +0100, Richard Biener wrote:
> >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.

So like this instead?  Bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?

2015-12-10  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/68730
	* cfgrtl.c (cfg_layout_finalize): Free dominators.

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

--- gcc/cfgrtl.c.jj	2015-11-04 11:12:20.000000000 +0100
+++ gcc/cfgrtl.c	2015-12-11 09:34:58.864893607 +0100
@@ -4299,6 +4299,7 @@ void
 cfg_layout_finalize (void)
 {
   checking_verify_flow_info ();
+  free_dominance_info (CDI_DOMINATORS);
   force_one_exit_fallthru ();
   rtl_register_cfg_hooks ();
   if (reload_completed && !targetm.have_epilogue ())
--- gcc/testsuite/gcc.dg/pr68730.c.jj	2015-12-11 09:25:06.022268146 +0100
+++ gcc/testsuite/gcc.dg/pr68730.c	2015-12-11 09:25:06.022268146 +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

Re: [PATCH] Fix ICE with -fno-if-conversion (PR rtl-optimization/68730)

[Jeff Law]

On 12/11/2015 01:13 PM, Jakub Jelinek wrote:
> On Fri, Dec 11, 2015 at 07:30:59AM +0100, Richard Biener wrote:
>>> 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.
>
> So like this instead?  Bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?
>
> 2015-12-10  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/68730
> 	* cfgrtl.c (cfg_layout_finalize): Free dominators.
>
> 	* gcc.dg/pr68730.c: New test.
OK.
jeff