Re: [PR103302] skip multi-word pre-move clobber during lra

[Richard Biener <richard.guenther@gmail.com>]

On Sat, Feb 19, 2022 at 12:28 AM Alexandre Oliva via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Dec 15, 2021, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
> >> * expr.c (emit_move_complex_parts): Skip clobbers during lra.
> > OK for the next cycle.
>
> Thanks, but having looked into PR 104121, I withdraw this patch and also
> the already-installed patch for PR 103302.  As I found out, LRA does
> worse without the clobbers for multi-word moves, not only because the
> clobbers shorten live ranges and enable earlier and better allocations,
> but also because find_reload_regno_insns implicitly, possibly
> unknowingly, relied on the clobbers to avoid the risk of an infinite
> loop.
>
> As noted in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104121#c11 with
> the clobber, a multi-word reload, and the insn the reload applies to, we
> get 4 insns, so find_reload_regno_insns avoids the loop.  Without the
> clobber, a multi-word reload for either input or output makes for n==3,
> so we enter the loop and don't ever exit it: we'll find first_insn
> (input) or second_insn (output), but then we'll loop forever because we
> won't iterate again on {prev,next}_insn, respectively, and the other
> iterator won't find the other word reload.  We advance the other till
> the end, but that's not enough for us to terminate the loop.
>
> With the proposed patch reversal, we no longer hit the problem with the
> v850 testcase in 104121, but I'm concerned we might still get an
> infinite loop on ports whose input or output reloads might emit a pair
> of insns without a clobber.
>
> I see two ways to robustify it.  One is to find a complete reload
> sequence:
>
> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
> index c1d40ea2a14bd..ff1688917cbce 100644
> --- a/gcc/lra-assigns.cc
> +++ b/gcc/lra-assigns.cc
> @@ -1716,9 +1716,18 @@ find_reload_regno_insns (int regno, rtx_insn * &start, rtx_insn * &finish)
>         start_insn = lra_insn_recog_data[uid]->insn;
>        n++;
>      }
> -  /* For reload pseudo we should have at most 3 insns referring for
> -     it: input/output reload insns and the original insn.  */
> -  if (n > 3)
> +  /* For reload pseudo we should have at most 3 (sequences of) insns
> +     referring for it: input/output reload insn sequences and the
> +     original insn.  Each reload insn sequence may amount to multiple
> +     insns, but we expect to find each of them contiguous, one before
> +     start_insn, one after it.  We know start_insn is between the
> +     sequences because it's the lowest-numbered insn, thus the first
> +     we'll have found above.  The reload insns, emitted later, will
> +     have been assigned higher insn uids.  If this assumption doesn't
> +     hold, and there happen to be intervening reload insns for other
> +     pseudos, we may end up returning false after searching to the end
> +     in the wrong direction.  */
> +  if (n > 1 + 2 * CEIL (lra_reg_info[regno].biggest_mode, UNITS_PER_WORD))
>      return false;
>    if (n > 1)
>      {
> @@ -1726,26 +1735,52 @@ find_reload_regno_insns (int regno, rtx_insn * &start, rtx_insn * &finish)
>              next_insn = NEXT_INSN (start_insn);
>            n != 1 && (prev_insn != NULL || next_insn != NULL); )
>         {
> -         if (prev_insn != NULL && first_insn == NULL)
> +         if (prev_insn != NULL)
>             {
>               if (! bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
>                                   INSN_UID (prev_insn)))
>                 prev_insn = PREV_INSN (prev_insn);
>               else
>                 {
> -                 first_insn = prev_insn;
> -                 n--;
> +                 /* A reload sequence may have multiple insns, but
> +                    they must be contiguous.  */
> +                 do
> +                   {
> +                     first_insn = prev_insn;
> +                     n--;
> +                     prev_insn = PREV_INSN (prev_insn);
> +                   }
> +                 while (n > 1 && prev_insn
> +                        && bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
> +                                         INSN_UID (prev_insn)));
> +                 /* After finding first_insn, we don't want to search
> +                    backward any more, so set prev_insn to NULL so as
> +                    to not loop indefinitely.  */
> +                 prev_insn = NULL;
>                 }
>             }
> -         if (next_insn != NULL && second_insn == NULL)
> +         else if (next_insn != NULL)
>             {
>               if (! bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
>                                 INSN_UID (next_insn)))
>                 next_insn = NEXT_INSN (next_insn);
>               else
>                 {
> -                 second_insn = next_insn;
> -                 n--;
> +                 /* A reload sequence may have multiple insns, but
> +                    they must be contiguous.  */
> +                 do
> +                   {
> +                     second_insn = next_insn;
> +                     n--;
> +                     next_insn = NEXT_INSN (next_insn);
> +                   }
> +                 while (n > 1 && next_insn
> +                        && bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
> +                                         INSN_UID (next_insn)));
> +                 /* After finding second_insn, we don't want to
> +                    search forward any more, so set next_insn to NULL
> +                    so as to not loop indefinitely.  */
> +                 next_insn = NULL;
>                 }
>             }
>         }
>
>
> the other is to just prevent the infinite loop, that will then return
> false because n > 1 after the loop ends:
>
> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
> index c1d40ea2a14bd..efd5f764a37a5 100644
> --- a/gcc/lra-assigns.cc
> +++ b/gcc/lra-assigns.cc
> @@ -1726,7 +1726,7 @@ find_reload_regno_insns (int regno, rtx_insn * &start, rtx_insn * &finish)
>              next_insn = NEXT_INSN (start_insn);
>            n != 1 && (prev_insn != NULL || next_insn != NULL); )
>         {
> -         if (prev_insn != NULL && first_insn == NULL)
> +         if (prev_insn != NULL)
>             {
>               if (! bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
>                                   INSN_UID (prev_insn)))
> @@ -1735,9 +1735,10 @@ find_reload_regno_insns (int regno, rtx_insn * &start, rtx_insn * &finish)
>                 {
>                   first_insn = prev_insn;
>                   n--;
> +                 prev_insn = NULL;
>                 }
>             }
> -         if (next_insn != NULL && second_insn == NULL)
> +         if (next_insn != NULL)
>             {
>               if (! bitmap_bit_p (&lra_reg_info[regno].insn_bitmap,
>                                 INSN_UID (next_insn)))
> @@ -1746,6 +1747,7 @@ find_reload_regno_insns (int regno, rtx_insn * &start, rtx_insn * &finish)
>                 {
>                   second_insn = next_insn;
>                   n--;
> +                 next_insn = NULL;
>                 }
>             }
>         }
>
>
> When it comes to the v850 testcase, one of them just moves the infinite
> loop to lra(), as we never get past while(fails_p); the other hits
> lra-assigns.cc:lra_assign (bool&)'s
>
>   if (flag_checking
>       && (lra_assignment_iter_after_spill
>           > LRA_MAX_ASSIGNMENT_ITERATION_NUMBER))
>     internal_error
>       ("maximum number of LRA assignment passes is achieved (%d)",
>        LRA_MAX_ASSIGNMENT_ITERATION_NUMBER);
>
> which would loop indefinitely too without flag_checking.
>
> Neither solves the v850 problem, only restoring the clobber does,
> because then, with shorter live ranges, allocation succeeds for the
> reloads, and we don't even try to split -> spill their pseudos.
>
> Would any of these patchlets make sense to pursue regardless?
>
> Ok to revert commit r12-5852-g50e8b0c9bca6cdc57804f860ec5311b641753fbb

OK.  Please re-open the bug as appropriate.

> I'm going to get back to the drawing board as to pr103302, since the
> problem there will likely resurface, possibly also on v850.
>
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 63a4aa838dec0..b6ed54983fabf 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -3929,7 +3929,7 @@ emit_move_multi_word (machine_mode mode, rtx x, rtx y)
>       hard regs shouldn't appear here except as return values.
>       We never want to emit such a clobber after reload.  */
>    if (x != y
> -      && ! (lra_in_progress || reload_in_progress || reload_completed)
> +      && ! (reload_in_progress || reload_completed)
>        && need_clobber != 0)
>      emit_clobber (x);
>
>
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>