On Mon, Dec 2, 2024 at 10:52 PM Andrew Pinski <quic_apin...@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_apin...@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
>
