On Tue, Dec 3, 2024 at 12:34 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> 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.
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
> >
