Re: PR rtl-optimization/49710 (segfault in loop peeling)

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Jan 4, 2012 at 2:01 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> in the testcase bellow the loop peeling simplifies exit condition in a way
> that the outer loop is destroyed. ?After some discussion with Zdenek I learnt
> that remove_path is then supposed to destroy the outer loop by calling unloop
> on it, but there is thinko in the function assuming that just the immediately
> outer loop can be eliminated. ?This is not true in this case: inner loop
> has two exists, one closing outer loop and the ohter closing outer loop of the
> outer loop. ?Outer loop stays, but outer loop of outer loop needs to be removed,
> so we need to walk the whole hiarchy upwards.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
> int a, b, c, d;
>
> static void
> foo (int *x)
> {
> ?c = 0;
> ?while (1)
> ? ?{
> ? ? ?if (*x)
> break;
> ? ? ?while (b)
> for (; c; c = 0);
> ? ? ?for (d = 18; d != 18; d++)
> if (c)
> ?{
> ? ?foo (x);
> ? ?return;
> ?}
> ? ?}
> }
>
> static void
> bar ()
> {
> ?foo (0);
> ?foo (0);
> ?for (;;)
> ? ?;
> }
>
> baz ()
> {
> ?for (; a;)
> ? ?bar ();
> }
> ? ? ? ?PR middle-end/49710
> ? ? ? ?* cfgloopmanip.c (remove_path): Walk loop hiearchy upwards when
> ? ? ? ?unlooping loops.
> Index: cfgloopmanip.c
> ===================================================================
> *** cfgloopmanip.c ? ? ?(revision 182871)
> --- cfgloopmanip.c ? ? ?(working copy)
> *************** remove_path (edge e)
> *** 291,296 ****
> --- 291,297 ----
> ? ?sbitmap seen;
> ? ?bool irred_invalidated = false;
> ? ?edge_iterator ei;
> + ? struct loop *l;
>
> ? ?if (!can_remove_branch_p (e))
> ? ? ?return false;
> *************** remove_path (edge e)
> *** 315,323 ****
> ? ? ? normally. ? We may assume that e->dest is not a header of any loop,
> ? ? ? as it now has exactly one predecessor. ?*/
> ? ?while (loop_outer (e->src->loop_father)
> ! ? ? ? ?&& dominated_by_p (CDI_DOMINATORS,
> ! ? ? ? ? ? ? ? ? ? ? ? ? ? e->src->loop_father->latch, e->dest))
> ? ? ?unloop (e->src->loop_father, &irred_invalidated);
>
> ? ?/* Identify the path. ?*/
> ? ?nrem = find_path (e, &rem_bbs);
> --- 316,333 ----
> ? ? ? normally. ? We may assume that e->dest is not a header of any loop,
> ? ? ? as it now has exactly one predecessor. ?*/
> ? ?while (loop_outer (e->src->loop_father)
> ! ? ? ? ? && dominated_by_p (CDI_DOMINATORS,
> ! ? ? ? ? ? ? ? ? ? ? ? ? ? ?e->src->loop_father->latch, e->dest))
> ? ? ?unloop (e->src->loop_father, &irred_invalidated);
> + ? l = e->src->loop_father;
> + ? while (l && loop_outer (l))
> + ? ? {
> + ? ? ? while (loop_outer (loop_outer (l))
> + ? ? ? ? ? ?&& dominated_by_p (CDI_DOMINATORS,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? loop_outer (l)->latch, e->dest))
> + ? ? ? ? unloop (loop_outer (l), &irred_invalidated);
> + ? ? ? l = loop_outer (l);
> + ? ? }

It must be possible to merge the two loops, no?  And it asks for a
comment.

> ? ?/* Identify the path. ?*/
> ? ?nrem = find_path (e, &rem_bbs);