[v2] phiopt: Reset the number of iterations information of a loop when changing an exit from the loop [PR117243]

Message ID 20241204052315.3854038-1-quic_apinski@quicinc.com
State New
Headers
Series [v2] phiopt: Reset the number of iterations information of a loop when changing an exit from the loop [PR117243] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

Andrew Pinski Dec. 4, 2024, 5:23 a.m. UTC
  After r12-5300-gf98f373dd822b3, phiopt could get the following bb structure:
      |
    middle-bb -----|
      |            |
      |   |----|   |
    phi<1, 2>  |   |
    cond       |   |
      |        |   |
      |--------+---|

Which was considered 2 loops. The inner loop had esimtate of upper_bound to be 8,
due to the original `for (b = 0; b <= 7; b++)`. The outer loop was already an
infinite one.
So phiopt would come along and change the condition to be unconditionally true,
we change the inner loop to being an infinite one but don't reset the estimate
on the loop and cleanup cfg comes along and changes it into one loop but also
does not reset the estimate of the loop. Then the loop unrolling uses the old estimate
and decides to add an unreachable there.o
So the fix is when phiopt changes an exit to a loop, reset the estimates, similar to
how cleanupcfg does it when merging some basic blocks.

Bootstrapped and tested on x86_64-linux-gnu.

	PR tree-optimization/117243
	PR tree-optimization/116749

gcc/ChangeLog:

	* tree-ssa-phiopt.cc (replace_phi_edge_with_variable): Reset loop
	estimates if the cond_block was an exit to a loop.

gcc/testsuite/ChangeLog:

	* gcc.dg/torture/pr117243-1.c: New test.
	* gcc.dg/torture/pr117243-2.c: New test.

Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
---
 gcc/testsuite/gcc.dg/torture/pr117243-1.c | 30 ++++++++++++++++++++
 gcc/testsuite/gcc.dg/torture/pr117243-2.c | 34 +++++++++++++++++++++++
 gcc/tree-ssa-phiopt.cc                    | 11 ++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-1.c
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-2.c
  

Comments

Richard Biener Dec. 4, 2024, 5:58 a.m. UTC | #1
> Am 04.12.2024 um 06:24 schrieb Andrew Pinski <quic_apinski@quicinc.com>:
> 
> After r12-5300-gf98f373dd822b3, phiopt could get the following bb structure:
>      |
>    middle-bb -----|
>      |            |
>      |   |----|   |
>    phi<1, 2>  |   |
>    cond       |   |
>      |        |   |
>      |--------+---|
> 
> Which was considered 2 loops. The inner loop had esimtate of upper_bound to be 8,
> due to the original `for (b = 0; b <= 7; b++)`. The outer loop was already an
> infinite one.
> So phiopt would come along and change the condition to be unconditionally true,
> we change the inner loop to being an infinite one but don't reset the estimate
> on the loop and cleanup cfg comes along and changes it into one loop but also
> does not reset the estimate of the loop. Then the loop unrolling uses the old estimate
> and decides to add an unreachable there.o
> So the fix is when phiopt changes an exit to a loop, reset the estimates, similar to
> how cleanupcfg does it when merging some basic blocks.
> 
> Bootstrapped and tested on x86_64-linux-gnu.

Ok

Richard 

>    PR tree-optimization/117243
>    PR tree-optimization/116749
> 
> gcc/ChangeLog:
> 
>    * tree-ssa-phiopt.cc (replace_phi_edge_with_variable): Reset loop
>    estimates if the cond_block was an exit to a loop.
> 
> gcc/testsuite/ChangeLog:
> 
>    * gcc.dg/torture/pr117243-1.c: New test.
>    * gcc.dg/torture/pr117243-2.c: New test.
> 
> Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>
> ---
> gcc/testsuite/gcc.dg/torture/pr117243-1.c | 30 ++++++++++++++++++++
> gcc/testsuite/gcc.dg/torture/pr117243-2.c | 34 +++++++++++++++++++++++
> gcc/tree-ssa-phiopt.cc                    | 11 ++++++++
> 3 files changed, 75 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-1.c
> create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-2.c
> 
> diff --git a/gcc/testsuite/gcc.dg/torture/pr117243-1.c b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
> new file mode 100644
> index 00000000000..c4bbc31467c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-tree-optimized" } */
> +/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
> +
> +/* PR tree-optimization/117243 */
> +/* foo should be an infinite but sometimes it gets optimized incorrectly into
> +   an __builtin_unreachable(); which is not valid.  */
> +void
> +foo (unsigned int a, unsigned char b)
> +{
> +  lbl:
> +  for (b = 0; b <= 7; b++)
> +    {
> +      unsigned char c[1][1];
> +      int i, j;
> +      for (i = 0; i < 1; i++)
> +        for (j = 0; j < 1; j++)
> +          c[i][j] = 1;
> +      if (b)
> +    goto lbl;
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  foo (1, 2);
> +}
> +
> +/* { dg-final { scan-tree-dump-not "__builtin_unreachable " "optimized"} } */
> diff --git a/gcc/testsuite/gcc.dg/torture/pr117243-2.c b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
> new file mode 100644
> index 00000000000..d9b0d3eeb98
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-tree-ch -fdump-tree-optimized" } */
> +/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
> +
> +/* PR tree-optimization/117243 */
> +/* PR tree-optimization/116749 */
> +
> +/* main1 should be an infinite but sometimes it gets optimized incorrectly into
> +   an __builtin_unreachable(); which is not valid.  */
> +int main1 (void)
> +{
> +    int g=0;
> +    int l1[1];
> +    int *l2 = &g;
> +    int i;
> +    for (i=0; i<1; i++)
> +        l1[i] = (1);
> +    for (g=0; g; ++g)
> +    {
> +        int *l3[1] = {&l1[0]};
> +    }
> +    *l2 = *l1;
> +b:
> +    for (i=0; i<2; ++i)
> +    {
> +        if (i)
> +            goto b;
> +        if (g)
> +            continue;
> +    }
> +    return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "__builtin_unreachable " "optimized"} } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 15651809d71..d7b7c7467c1 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3.  If not see
> #include "tree-ssa-propagate.h"
> #include "tree-ssa-dce.h"
> #include "calls.h"
> +#include "tree-ssa-loop-niter.h"
> 
> /* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
> 
> @@ -152,6 +153,16 @@ replace_phi_edge_with_variable (basic_block cond_block,
>   else
>     gcc_unreachable ();
> 
> +  /* If we are removing the cond on a loop exit,
> +     reset number of iteration information of the loop. */
> +  if (loop_exits_from_bb_p (cond_block->loop_father, cond_block))
> +    {
> +      auto loop = cond_block->loop_father;
> +      free_numbers_of_iterations_estimates (loop);
> +      loop->any_upper_bound = false;
> +      loop->any_likely_upper_bound = false;
> +    }
> +
>   if (edge_to_remove && EDGE_COUNT (edge_to_remove->dest->preds) == 1)
>     {
>       e->flags |= EDGE_FALLTHRU;
> --
> 2.43.0
>
  

Patch

diff --git a/gcc/testsuite/gcc.dg/torture/pr117243-1.c b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
new file mode 100644
index 00000000000..c4bbc31467c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
@@ -0,0 +1,30 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdump-tree-optimized" } */
+/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+
+/* PR tree-optimization/117243 */
+/* foo should be an infinite but sometimes it gets optimized incorrectly into
+   an __builtin_unreachable(); which is not valid.  */
+void
+foo (unsigned int a, unsigned char b)
+{
+  lbl:
+  for (b = 0; b <= 7; b++)
+    {
+      unsigned char c[1][1];
+      int i, j;
+      for (i = 0; i < 1; i++)
+        for (j = 0; j < 1; j++)
+          c[i][j] = 1;
+      if (b)
+	goto lbl;
+    }
+}
+
+int
+main ()
+{
+  foo (1, 2);
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_unreachable " "optimized"} } */
diff --git a/gcc/testsuite/gcc.dg/torture/pr117243-2.c b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
new file mode 100644
index 00000000000..d9b0d3eeb98
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
@@ -0,0 +1,34 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fno-tree-ch -fdump-tree-optimized" } */
+/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+
+/* PR tree-optimization/117243 */
+/* PR tree-optimization/116749 */
+
+/* main1 should be an infinite but sometimes it gets optimized incorrectly into
+   an __builtin_unreachable(); which is not valid.  */
+int main1 (void)
+{
+    int g=0;
+    int l1[1];
+    int *l2 = &g;
+    int i;
+    for (i=0; i<1; i++)
+        l1[i] = (1);
+    for (g=0; g; ++g)
+    {
+        int *l3[1] = {&l1[0]};
+    }
+    *l2 = *l1;
+b:
+    for (i=0; i<2; ++i)
+    { 
+        if (i)
+            goto b;
+        if (g)
+            continue;
+    }
+    return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_unreachable " "optimized"} } */
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 15651809d71..d7b7c7467c1 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -55,6 +55,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "tree-ssa-dce.h"
 #include "calls.h"
+#include "tree-ssa-loop-niter.h"
 
 /* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
 
@@ -152,6 +153,16 @@  replace_phi_edge_with_variable (basic_block cond_block,
   else
     gcc_unreachable ();
 
+  /* If we are removing the cond on a loop exit,
+     reset number of iteration information of the loop. */
+  if (loop_exits_from_bb_p (cond_block->loop_father, cond_block))
+    {
+      auto loop = cond_block->loop_father;
+      free_numbers_of_iterations_estimates (loop);
+      loop->any_upper_bound = false;
+      loop->any_likely_upper_bound = false;
+    }
+
   if (edge_to_remove && EDGE_COUNT (edge_to_remove->dest->preds) == 1)
     {
       e->flags |= EDGE_FALLTHRU;