[PATCH, PR rtl-optimization/71148] Avoid cleanup_cfg called with invalidated dominance info

[PATCH, PR rtl-optimization/71148] Avoid cleanup_cfg called with invalidated dominance info

[Ilya Enkovich]

Hi,

This patch resolves PR71148 by releasing dominance info before
cleanup_cfg calls to avoid attempts to fixup invalid dominance
info.

Dominance info handling in cleanup_cfg looks weird though.  It
tries to fix it but can invalidate it at the same time (PR71084).
We should probably do something with that.

Tracker is P1 and this patch may be OK solution for now.

Bootstrapped and regtested on x86_64-pc-linux-gnu.  Ok for trunk?

Thanks,
Ilya
--
gcc/

2016-05-18  Ilya Enkovich  <ilya.enkovich@intel.com>

	PR rtl-optimization/71148
	* cse.c (rest_of_handle_cse): Free dominance info
	before cleanup_cfg call if required.
	(rest_of_handle_cse2): Likewise.
	(rest_of_handle_cse_after_global_opts): Likewise.

gcc/testsuite/

2016-05-18  Ilya Enkovich  <ilya.enkovich@intel.com>

	PR rtl-optimization/71148
	* gcc.dg/pr71148.c: New test.


diff --git a/gcc/cse.c b/gcc/cse.c
index 322e352..4aa4443 100644
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -7558,6 +7558,12 @@ rest_of_handle_cse (void)
      expecting CSE to be run.  But always rerun it in a cheap mode.  */
   cse_not_expected = !flag_rerun_cse_after_loop && !flag_gcse;
 
+  /* Check if we need to free dominance info before cleanup_cfg
+     because it may become really slow in case of invalid
+     dominance info.  */
+  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
+    free_dominance_info (CDI_DOMINATORS);
+
   if (tem == 2)
     {
       timevar_push (TV_JUMP);
@@ -7630,6 +7636,12 @@ rest_of_handle_cse2 (void)
 
   delete_trivially_dead_insns (get_insns (), max_reg_num ());
 
+  /* Check if we need to free dominance info before cleanup_cfg
+     because it may become really slow in case of invalid
+     dominance info.  */
+  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
+    free_dominance_info (CDI_DOMINATORS);
+
   if (tem == 2)
     {
       timevar_push (TV_JUMP);
@@ -7706,6 +7718,12 @@ rest_of_handle_cse_after_global_opts (void)
 
   cse_not_expected = !flag_rerun_cse_after_loop;
 
+  /* Check if we need to free dominance info before cleanup_cfg
+     because it may become really slow in case of invalid
+     dominance info.  */
+  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
+    free_dominance_info (CDI_DOMINATORS);
+
   /* If cse altered any jumps, rerun jump opts to clean things up.  */
   if (tem == 2)
     {
diff --git a/gcc/testsuite/gcc.dg/pr71148.c b/gcc/testsuite/gcc.dg/pr71148.c
new file mode 100644
index 0000000..6aa4920
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr71148.c
@@ -0,0 +1,46 @@
+/* PR rtl-optimization/71148 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -funroll-loops" } */
+
+int rh, ok, kq, fu;
+
+void
+js (int cs)
+{
+  rh = fu;
+  if (fu != 0)
+    {
+      cs /= 3;
+      if (cs <= 0)
+        {
+          int z9;
+          for (z9 = 0; z9 < 2; ++z9)
+            {
+              z9 += cs;
+              ok += z9;
+              fu += ok;
+            }
+        }
+    }
+}
+
+void
+vy (int s3)
+{
+  int yo, g2 = 0;
+ sd:
+  js (g2);
+  for (yo = 0; yo < 2; ++yo)
+    {
+      if (fu != 0)
+        goto sd;
+      kq += (s3 != (g2 ? s3 : 0));
+      for (s3 = 0; s3 < 72; ++s3)
+        g2 *= (~0 - 1);
+      g2 -= yo;
+    }
+  for (fu = 0; fu < 18; ++fu)
+    for (yo = 0; yo < 17; ++yo)
+      if (g2 < 0)
+        goto sd;
+}

Re: [PATCH, PR rtl-optimization/71148] Avoid cleanup_cfg called with invalidated dominance info

[Richard Biener]

On Wed, May 18, 2016 at 6:17 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> Hi,
>
> This patch resolves PR71148 by releasing dominance info before
> cleanup_cfg calls to avoid attempts to fixup invalid dominance
> info.
>
> Dominance info handling in cleanup_cfg looks weird though.  It
> tries to fix it but can invalidate it at the same time (PR71084).
> We should probably do something with that.
>
> Tracker is P1 and this patch may be OK solution for now.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu.  Ok for trunk?

Hum... all this dancing with CSE looks bogus to me.  Does CSE itself
need dominance info?  If not then unconditionally free it at its start.

Richard.

> Thanks,
> Ilya
> --
> gcc/
>
> 2016-05-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         PR rtl-optimization/71148
>         * cse.c (rest_of_handle_cse): Free dominance info
>         before cleanup_cfg call if required.
>         (rest_of_handle_cse2): Likewise.
>         (rest_of_handle_cse_after_global_opts): Likewise.
>
> gcc/testsuite/
>
> 2016-05-18  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         PR rtl-optimization/71148
>         * gcc.dg/pr71148.c: New test.
>
>
> diff --git a/gcc/cse.c b/gcc/cse.c
> index 322e352..4aa4443 100644
> --- a/gcc/cse.c
> +++ b/gcc/cse.c
> @@ -7558,6 +7558,12 @@ rest_of_handle_cse (void)
>       expecting CSE to be run.  But always rerun it in a cheap mode.  */
>    cse_not_expected = !flag_rerun_cse_after_loop && !flag_gcse;
>
> +  /* Check if we need to free dominance info before cleanup_cfg
> +     because it may become really slow in case of invalid
> +     dominance info.  */
> +  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> +    free_dominance_info (CDI_DOMINATORS);
> +
>    if (tem == 2)
>      {
>        timevar_push (TV_JUMP);
> @@ -7630,6 +7636,12 @@ rest_of_handle_cse2 (void)
>
>    delete_trivially_dead_insns (get_insns (), max_reg_num ());
>
> +  /* Check if we need to free dominance info before cleanup_cfg
> +     because it may become really slow in case of invalid
> +     dominance info.  */
> +  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> +    free_dominance_info (CDI_DOMINATORS);
> +
>    if (tem == 2)
>      {
>        timevar_push (TV_JUMP);
> @@ -7706,6 +7718,12 @@ rest_of_handle_cse_after_global_opts (void)
>
>    cse_not_expected = !flag_rerun_cse_after_loop;
>
> +  /* Check if we need to free dominance info before cleanup_cfg
> +     because it may become really slow in case of invalid
> +     dominance info.  */
> +  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> +    free_dominance_info (CDI_DOMINATORS);
> +
>    /* If cse altered any jumps, rerun jump opts to clean things up.  */
>    if (tem == 2)
>      {
> diff --git a/gcc/testsuite/gcc.dg/pr71148.c b/gcc/testsuite/gcc.dg/pr71148.c
> new file mode 100644
> index 0000000..6aa4920
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr71148.c
> @@ -0,0 +1,46 @@
> +/* PR rtl-optimization/71148 */
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -funroll-loops" } */
> +
> +int rh, ok, kq, fu;
> +
> +void
> +js (int cs)
> +{
> +  rh = fu;
> +  if (fu != 0)
> +    {
> +      cs /= 3;
> +      if (cs <= 0)
> +        {
> +          int z9;
> +          for (z9 = 0; z9 < 2; ++z9)
> +            {
> +              z9 += cs;
> +              ok += z9;
> +              fu += ok;
> +            }
> +        }
> +    }
> +}
> +
> +void
> +vy (int s3)
> +{
> +  int yo, g2 = 0;
> + sd:
> +  js (g2);
> +  for (yo = 0; yo < 2; ++yo)
> +    {
> +      if (fu != 0)
> +        goto sd;
> +      kq += (s3 != (g2 ? s3 : 0));
> +      for (s3 = 0; s3 < 72; ++s3)
> +        g2 *= (~0 - 1);
> +      g2 -= yo;
> +    }
> +  for (fu = 0; fu < 18; ++fu)
> +    for (yo = 0; yo < 17; ++yo)
> +      if (g2 < 0)
> +        goto sd;
> +}

Re: [PATCH, PR rtl-optimization/71148] Avoid cleanup_cfg called with invalidated dominance info

[Ilya Enkovich]

On 19 May 09:57, Richard Biener wrote:
> On Wed, May 18, 2016 at 6:17 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> > Hi,
> >
> > This patch resolves PR71148 by releasing dominance info before
> > cleanup_cfg calls to avoid attempts to fixup invalid dominance
> > info.
> >
> > Dominance info handling in cleanup_cfg looks weird though.  It
> > tries to fix it but can invalidate it at the same time (PR71084).
> > We should probably do something with that.
> >
> > Tracker is P1 and this patch may be OK solution for now.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu.  Ok for trunk?
> 
> Hum... all this dancing with CSE looks bogus to me.  Does CSE itself
> need dominance info?  If not then unconditionally free it at its start.
> 
> Richard.
> 

Here it is.  OK if testing pass?

Thanks,
Ilya
--
gcc/

2016-05-19  Ilya Enkovich  <ilya.enkovich@intel.com>

	* cse.c (cse_main): Free dominance info.
	(rest_of_handle_cse): Don't free dominance info.
	(rest_of_handle_cse2): Likewise.
	(rest_of_handle_cse_after_global_opts): Likewise.

gcc/testsuite/

2016-05-19  Ilya Enkovich  <ilya.enkovich@intel.com>

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


diff --git a/gcc/cse.c b/gcc/cse.c
index 322e352..84082fb 100644
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -6669,6 +6669,11 @@ cse_main (rtx_insn *f ATTRIBUTE_UNUSED, int nregs)
   int *rc_order = XNEWVEC (int, last_basic_block_for_fn (cfun));
   int i, n_blocks;
 
+  /* CSE doesn't use dominance info but can invalidate it in different ways.
+     For simplicity free dominance info here.  */
+  if (dom_info_available_p (CDI_DOMINATORS))
+    free_dominance_info (CDI_DOMINATORS);
+
   df_set_flags (DF_LR_RUN_DCE);
   df_note_add_problem ();
   df_analyze ();
@@ -7568,9 +7573,6 @@ rest_of_handle_cse (void)
   else if (tem == 1 || optimize > 1)
     cse_cfg_altered |= cleanup_cfg (0);
 
-  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
-    free_dominance_info (CDI_DOMINATORS);
-
   return 0;
 }
 
@@ -7640,9 +7642,6 @@ rest_of_handle_cse2 (void)
   else if (tem == 1)
     cse_cfg_altered |= cleanup_cfg (0);
 
-  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
-    free_dominance_info (CDI_DOMINATORS);
-
   cse_not_expected = 1;
   return 0;
 }
@@ -7717,9 +7716,6 @@ rest_of_handle_cse_after_global_opts (void)
   else if (tem == 1)
     cse_cfg_altered |= cleanup_cfg (0);
 
-  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
-    free_dominance_info (CDI_DOMINATORS);
-
   flag_cse_follow_jumps = save_cfj;
   return 0;
 }
diff --git a/gcc/testsuite/gcc.dg/pr71148.c b/gcc/testsuite/gcc.dg/pr71148.c
new file mode 100644
index 0000000..6aa4920
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr71148.c
@@ -0,0 +1,46 @@
+/* PR rtl-optimization/71148 */
+/* { dg-do compile } */
+/* { dg-options "-O3 -funroll-loops" } */
+
+int rh, ok, kq, fu;
+
+void
+js (int cs)
+{
+  rh = fu;
+  if (fu != 0)
+    {
+      cs /= 3;
+      if (cs <= 0)
+        {
+          int z9;
+          for (z9 = 0; z9 < 2; ++z9)
+            {
+              z9 += cs;
+              ok += z9;
+              fu += ok;
+            }
+        }
+    }
+}
+
+void
+vy (int s3)
+{
+  int yo, g2 = 0;
+ sd:
+  js (g2);
+  for (yo = 0; yo < 2; ++yo)
+    {
+      if (fu != 0)
+        goto sd;
+      kq += (s3 != (g2 ? s3 : 0));
+      for (s3 = 0; s3 < 72; ++s3)
+        g2 *= (~0 - 1);
+      g2 -= yo;
+    }
+  for (fu = 0; fu < 18; ++fu)
+    for (yo = 0; yo < 17; ++yo)
+      if (g2 < 0)
+        goto sd;
+}

Re: [PATCH, PR rtl-optimization/71148] Avoid cleanup_cfg called with invalidated dominance info

[Richard Biener]

On Thu, May 19, 2016 at 12:34 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> On 19 May 09:57, Richard Biener wrote:
>> On Wed, May 18, 2016 at 6:17 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> > Hi,
>> >
>> > This patch resolves PR71148 by releasing dominance info before
>> > cleanup_cfg calls to avoid attempts to fixup invalid dominance
>> > info.
>> >
>> > Dominance info handling in cleanup_cfg looks weird though.  It
>> > tries to fix it but can invalidate it at the same time (PR71084).
>> > We should probably do something with that.
>> >
>> > Tracker is P1 and this patch may be OK solution for now.
>> >
>> > Bootstrapped and regtested on x86_64-pc-linux-gnu.  Ok for trunk?
>>
>> Hum... all this dancing with CSE looks bogus to me.  Does CSE itself
>> need dominance info?  If not then unconditionally free it at its start.
>>
>> Richard.
>>
>
> Here it is.  OK if testing pass?
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2016-05-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * cse.c (cse_main): Free dominance info.
>         (rest_of_handle_cse): Don't free dominance info.
>         (rest_of_handle_cse2): Likewise.
>         (rest_of_handle_cse_after_global_opts): Likewise.
>
> gcc/testsuite/
>
> 2016-05-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>
>         * gcc.dg/pr71148.c: New test.
>
>
> diff --git a/gcc/cse.c b/gcc/cse.c
> index 322e352..84082fb 100644
> --- a/gcc/cse.c
> +++ b/gcc/cse.c
> @@ -6669,6 +6669,11 @@ cse_main (rtx_insn *f ATTRIBUTE_UNUSED, int nregs)
>    int *rc_order = XNEWVEC (int, last_basic_block_for_fn (cfun));
>    int i, n_blocks;
>
> +  /* CSE doesn't use dominance info but can invalidate it in different ways.
> +     For simplicity free dominance info here.  */
> +  if (dom_info_available_p (CDI_DOMINATORS))
> +    free_dominance_info (CDI_DOMINATORS);

Just call free_dominance_info unconditionally.

Ok with that change (if it passes testing).

Richard.

> +
>    df_set_flags (DF_LR_RUN_DCE);
>    df_note_add_problem ();
>    df_analyze ();
> @@ -7568,9 +7573,6 @@ rest_of_handle_cse (void)
>    else if (tem == 1 || optimize > 1)
>      cse_cfg_altered |= cleanup_cfg (0);
>
> -  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> -    free_dominance_info (CDI_DOMINATORS);
> -
>    return 0;
>  }
>
> @@ -7640,9 +7642,6 @@ rest_of_handle_cse2 (void)
>    else if (tem == 1)
>      cse_cfg_altered |= cleanup_cfg (0);
>
> -  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> -    free_dominance_info (CDI_DOMINATORS);
> -
>    cse_not_expected = 1;
>    return 0;
>  }
> @@ -7717,9 +7716,6 @@ rest_of_handle_cse_after_global_opts (void)
>    else if (tem == 1)
>      cse_cfg_altered |= cleanup_cfg (0);
>
> -  if (cse_cfg_altered && dom_info_available_p (CDI_DOMINATORS))
> -    free_dominance_info (CDI_DOMINATORS);
> -
>    flag_cse_follow_jumps = save_cfj;
>    return 0;
>  }
> diff --git a/gcc/testsuite/gcc.dg/pr71148.c b/gcc/testsuite/gcc.dg/pr71148.c
> new file mode 100644
> index 0000000..6aa4920
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr71148.c
> @@ -0,0 +1,46 @@
> +/* PR rtl-optimization/71148 */
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -funroll-loops" } */
> +
> +int rh, ok, kq, fu;
> +
> +void
> +js (int cs)
> +{
> +  rh = fu;
> +  if (fu != 0)
> +    {
> +      cs /= 3;
> +      if (cs <= 0)
> +        {
> +          int z9;
> +          for (z9 = 0; z9 < 2; ++z9)
> +            {
> +              z9 += cs;
> +              ok += z9;
> +              fu += ok;
> +            }
> +        }
> +    }
> +}
> +
> +void
> +vy (int s3)
> +{
> +  int yo, g2 = 0;
> + sd:
> +  js (g2);
> +  for (yo = 0; yo < 2; ++yo)
> +    {
> +      if (fu != 0)
> +        goto sd;
> +      kq += (s3 != (g2 ? s3 : 0));
> +      for (s3 = 0; s3 < 72; ++s3)
> +        g2 *= (~0 - 1);
> +      g2 -= yo;
> +    }
> +  for (fu = 0; fu < 18; ++fu)
> +    for (yo = 0; yo < 17; ++yo)
> +      if (g2 < 0)
> +        goto sd;
> +}