phiopt: don't handle the case cond edge dest is itself [PR117243]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
Commit Message
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
and cleanup cfg would remove the condition and remove the outer loop but not
update the inner one becoming an infinite loop.
I decided it was easier to avoid this inside phiopt rather than figuring out how
to fix up cleanup cfg.
This patch avoids the issue by rejecting edges back to the condition bb before
loop optmiizations have been run.
Bootstrapped and tested on x86_64-linux-gnu.
Note since the testcases depend on the loop being infinite, I used the alarm signal trick.
PR tree-optimization/117243
PR tree-optimization/116749
gcc/ChangeLog:
* tree-ssa-phiopt.cc (execute_over_cond_phis): Reject edges back
to the conditional bb before loop optimizers have been run.
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 | 44 ++++++++++++++++++
gcc/testsuite/gcc.dg/torture/pr117243-2.c | 54 +++++++++++++++++++++++
gcc/tree-ssa-phiopt.cc | 7 +++
3 files changed, 105 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-1.c
create mode 100644 gcc/testsuite/gcc.dg/torture/pr117243-2.c
Comments
On Mon, Dec 02, 2024 at 01:51:39PM -0800, Andrew Pinski wrote:
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/torture/pr117243-1.c: New test.
> * gcc.dg/torture/pr117243-2.c: New test.
Just commenting on the testcases.
I don't like 2 tests times 7 different command line options each spend
some time in busy loop uselessly.
Can't you just make it dg-do compile and scan for optimized dump not having
__builtin_unreachable calls?
Jakub
On Mon, Dec 2, 2024 at 2:05 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Dec 02, 2024 at 01:51:39PM -0800, Andrew Pinski wrote:
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/torture/pr117243-1.c: New test.
> > * gcc.dg/torture/pr117243-2.c: New test.
>
> Just commenting on the testcases.
> I don't like 2 tests times 7 different command line options each spend
> some time in busy loop uselessly.
> Can't you just make it dg-do compile and scan for optimized dump not having
> __builtin_unreachable calls?
I guess that would work better really. I will rewrite testcases to do that.
THanks,
Andrew
>
> Jakub
>
On Mon, Dec 2, 2024 at 10:52 PM Andrew Pinski <quic_apinski@quicinc.com> wrote:
>
> 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
> and cleanup cfg would remove the condition and remove the outer loop but not
> update the inner one becoming an infinite loop.
> I decided it was easier to avoid this inside phiopt rather than figuring out how
> to fix up cleanup cfg.
>
> This patch avoids the issue by rejecting edges back to the condition bb before
> loop optmiizations have been run.
>
> Bootstrapped and tested on x86_64-linux-gnu.
>
> Note since the testcases depend on the loop being infinite, I used the alarm signal trick.
>
> PR tree-optimization/117243
> PR tree-optimization/116749
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (execute_over_cond_phis): Reject edges back
> to the conditional bb before loop optimizers have been run.
>
> 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 | 44 ++++++++++++++++++
> gcc/testsuite/gcc.dg/torture/pr117243-2.c | 54 +++++++++++++++++++++++
> gcc/tree-ssa-phiopt.cc | 7 +++
> 3 files changed, 105 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..46723132553
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
> @@ -0,0 +1,44 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target signal } */
> +
> +/* PR tree-optimization/117243 */
> +#include <unistd.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +
> +void do_exit (int i)
> +{
> + exit (0);
> +}
> +
> +/* 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 ()
> +{
> + struct sigaction s;
> + sigemptyset (&s.sa_mask);
> + s.sa_handler = do_exit;
> + s.sa_flags = 0;
> + sigaction (SIGALRM, &s, NULL);
> + alarm (1);
> +
> + foo (1, 2);
> +}
> +
> 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..5cb864b467d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
> @@ -0,0 +1,54 @@
> +/* { dg-do run } */
> +/* { dg-additional-options "-fno-tree-ch" } */
> +/* { dg-require-effective-target signal } */
> +
> +/* PR tree-optimization/117243 */
> +/* PR tree-optimization/116749 */
> +#include <unistd.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +
> +void do_exit (int i)
> +{
> + exit (0);
> +}
> +
> +/* 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;
> +}
> +
> +int
> +main (void)
> +{
> + struct sigaction s;
> + sigemptyset (&s.sa_mask);
> + s.sa_handler = do_exit;
> + s.sa_flags = 0;
> + sigaction (SIGALRM, &s, NULL);
> + alarm (1);
> +
> + main1 ();
> +}
> +
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 15651809d71..a2649590d29 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -4178,6 +4178,13 @@ execute_over_cond_phis (func_type func)
> e2 = EDGE_SUCC (bb, 1);
> bb2 = e2->dest;
>
> + /* If loop opts are not done, then don't handle a loop back to itself.
> + The loop estimates sometimes are not updated correctly when changing
> + a loop into an infinite loop. */
> + if (!(cfun->curr_properties & PROP_loop_opts_done)
> + && (bb1 == bb || bb2 == bb))
> + continue;
It seems incomplete if you think of a forwarder between e1/2->dest and BB.
I'd say you'd want to disallow dominated_by_p (CDI_DOMINATORS, bb, bb1/2),
aka backedges. And why only when !PROP_loop_opts_done?
Indeed CFG cleanup is supposed to handle all sorts of required loop updates here
but for the case in question phiopt changes the exit condition of a
loop and thus
phiopt is responsible for clearing loop estimates. So another fix
would be to simply
never process loop exit conditions (loop_exits_from_bb_p (bb->loop_father, bb)),
or if such a condition is altered, call free_numbers_of_iterations_estimates
on the loop and reset ->any_upper_bound and ->any_likely_upper_bound.
Richard.
> +
> /* We cannot do the optimization on abnormal edges. */
> if ((e1->flags & EDGE_ABNORMAL) != 0
> || (e2->flags & EDGE_ABNORMAL) != 0)
> --
> 2.43.0
>
On Tue, Dec 3, 2024 at 12:34 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Dec 2, 2024 at 10:52 PM Andrew Pinski <quic_apinski@quicinc.com> wrote:
> >
> > 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
> > and cleanup cfg would remove the condition and remove the outer loop but not
> > update the inner one becoming an infinite loop.
> > I decided it was easier to avoid this inside phiopt rather than figuring out how
> > to fix up cleanup cfg.
> >
> > This patch avoids the issue by rejecting edges back to the condition bb before
> > loop optmiizations have been run.
> >
> > Bootstrapped and tested on x86_64-linux-gnu.
> >
> > Note since the testcases depend on the loop being infinite, I used the alarm signal trick.
> >
> > PR tree-optimization/117243
> > PR tree-optimization/116749
> >
> > gcc/ChangeLog:
> >
> > * tree-ssa-phiopt.cc (execute_over_cond_phis): Reject edges back
> > to the conditional bb before loop optimizers have been run.
> >
> > 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 | 44 ++++++++++++++++++
> > gcc/testsuite/gcc.dg/torture/pr117243-2.c | 54 +++++++++++++++++++++++
> > gcc/tree-ssa-phiopt.cc | 7 +++
> > 3 files changed, 105 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..46723132553
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr117243-1.c
> > @@ -0,0 +1,44 @@
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target signal } */
> > +
> > +/* PR tree-optimization/117243 */
> > +#include <unistd.h>
> > +#include <signal.h>
> > +#include <stdlib.h>
> > +
> > +void do_exit (int i)
> > +{
> > + exit (0);
> > +}
> > +
> > +/* 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 ()
> > +{
> > + struct sigaction s;
> > + sigemptyset (&s.sa_mask);
> > + s.sa_handler = do_exit;
> > + s.sa_flags = 0;
> > + sigaction (SIGALRM, &s, NULL);
> > + alarm (1);
> > +
> > + foo (1, 2);
> > +}
> > +
> > 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..5cb864b467d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr117243-2.c
> > @@ -0,0 +1,54 @@
> > +/* { dg-do run } */
> > +/* { dg-additional-options "-fno-tree-ch" } */
> > +/* { dg-require-effective-target signal } */
> > +
> > +/* PR tree-optimization/117243 */
> > +/* PR tree-optimization/116749 */
> > +#include <unistd.h>
> > +#include <signal.h>
> > +#include <stdlib.h>
> > +
> > +void do_exit (int i)
> > +{
> > + exit (0);
> > +}
> > +
> > +/* 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;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > + struct sigaction s;
> > + sigemptyset (&s.sa_mask);
> > + s.sa_handler = do_exit;
> > + s.sa_flags = 0;
> > + sigaction (SIGALRM, &s, NULL);
> > + alarm (1);
> > +
> > + main1 ();
> > +}
> > +
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index 15651809d71..a2649590d29 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -4178,6 +4178,13 @@ execute_over_cond_phis (func_type func)
> > e2 = EDGE_SUCC (bb, 1);
> > bb2 = e2->dest;
> >
> > + /* If loop opts are not done, then don't handle a loop back to itself.
> > + The loop estimates sometimes are not updated correctly when changing
> > + a loop into an infinite loop. */
> > + if (!(cfun->curr_properties & PROP_loop_opts_done)
> > + && (bb1 == bb || bb2 == bb))
> > + continue;
>
> It seems incomplete if you think of a forwarder between e1/2->dest and BB.
> I'd say you'd want to disallow dominated_by_p (CDI_DOMINATORS, bb, bb1/2),
> aka backedges. And why only when !PROP_loop_opts_done?
>
> Indeed CFG cleanup is supposed to handle all sorts of required loop updates here
> but for the case in question phiopt changes the exit condition of a
> loop and thus
> phiopt is responsible for clearing loop estimates. So another fix
> would be to simply
> never process loop exit conditions (loop_exits_from_bb_p (bb->loop_father, bb)),
> or if such a condition is altered, call free_numbers_of_iterations_estimates
> on the loop and reset ->any_upper_bound and ->any_likely_upper_bound.
Oh makes sense. I have a patch where if a condition for a loop exit is
going to be removed,
call free_numbers_of_iterations_estimates, and reset
any_upper_bound/any_likely_upper_bound on the loop and that fixes the
issue.
It is actually easy to add that in phiopt since the place which
changes the condition to true/false (or removes the bb) is
replace_phi_edge_with_variable.
Also I updated the testcase to be a compile time one instead of a run
one as recommended by Jakub.
Than ks,
Andrew Pinski
>
> Richard.
>
> > +
> > /* We cannot do the optimization on abnormal edges. */
> > if ((e1->flags & EDGE_ABNORMAL) != 0
> > || (e2->flags & EDGE_ABNORMAL) != 0)
> > --
> > 2.43.0
> >
new file mode 100644
@@ -0,0 +1,44 @@
+/* { dg-do run } */
+/* { dg-require-effective-target signal } */
+
+/* PR tree-optimization/117243 */
+#include <unistd.h>
+#include <signal.h>
+#include <stdlib.h>
+
+void do_exit (int i)
+{
+ exit (0);
+}
+
+/* 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 ()
+{
+ struct sigaction s;
+ sigemptyset (&s.sa_mask);
+ s.sa_handler = do_exit;
+ s.sa_flags = 0;
+ sigaction (SIGALRM, &s, NULL);
+ alarm (1);
+
+ foo (1, 2);
+}
+
new file mode 100644
@@ -0,0 +1,54 @@
+/* { dg-do run } */
+/* { dg-additional-options "-fno-tree-ch" } */
+/* { dg-require-effective-target signal } */
+
+/* PR tree-optimization/117243 */
+/* PR tree-optimization/116749 */
+#include <unistd.h>
+#include <signal.h>
+#include <stdlib.h>
+
+void do_exit (int i)
+{
+ exit (0);
+}
+
+/* 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;
+}
+
+int
+main (void)
+{
+ struct sigaction s;
+ sigemptyset (&s.sa_mask);
+ s.sa_handler = do_exit;
+ s.sa_flags = 0;
+ sigaction (SIGALRM, &s, NULL);
+ alarm (1);
+
+ main1 ();
+}
+
@@ -4178,6 +4178,13 @@ execute_over_cond_phis (func_type func)
e2 = EDGE_SUCC (bb, 1);
bb2 = e2->dest;
+ /* If loop opts are not done, then don't handle a loop back to itself.
+ The loop estimates sometimes are not updated correctly when changing
+ a loop into an infinite loop. */
+ if (!(cfun->curr_properties & PROP_loop_opts_done)
+ && (bb1 == bb || bb2 == bb))
+ continue;
+
/* We cannot do the optimization on abnormal edges. */
if ((e1->flags & EDGE_ABNORMAL) != 0
|| (e2->flags & EDGE_ABNORMAL) != 0)