Re: move unreachable user labels to entry point

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

On Tue, Jul 13, 2021 at 5:11 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> pr42739.C, complicated by some extra wrappers and cleanups from a
> feature I'm working on, got me very confused because a user label
> ended up in a cleanup introduced by my pass, where it couldn't
> possibly have been initially.
>
> The current logic may move such an unreachable user label multiple
> times, if it ends up in blocks that get removed one after the other.
> Since it doesn't really matter where it lands (save for omp
> constraints), I propose we move it once and for all to a stable, final
> location, that we currently use only as a last resort: the entry
> point.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?
>
>
> for  gcc/ChangeLog
>
>         * tree-cfg.c (remove_bb): When preserving an unreachable user
>         label, use the entry block as the preferred choice for its
>         surviving location, rather than as a last resort.
> ---
>  gcc/tree-cfg.c |   18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 1f0f4a2c6eb2c..f6f005f10a9f5 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -2300,13 +2300,11 @@ remove_bb (basic_block bb)
>                   FORCED_LABEL (gimple_label_label (label_stmt)) = 1;
>                 }
>
> -             new_bb = bb->prev_bb;
> -             /* Don't move any labels into ENTRY block.  */
> -             if (new_bb == ENTRY_BLOCK_PTR_FOR_FN (cfun))
> -               {
> -                 new_bb = single_succ (new_bb);
> -                 gcc_assert (new_bb != bb);
> -               }
> +             /* We have to move the unreachable label somewhere.
> +                Moving it to the entry block makes sure it's moved at
> +                most once, and avoids messing with anonymous landing
> +                pad labels.  */
> +             new_bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>               if ((unsigned) bb->index < bb_to_omp_idx.length ()
>                   && ((unsigned) new_bb->index >= bb_to_omp_idx.length ()
>                       || (bb_to_omp_idx[bb->index]
> @@ -2316,7 +2314,6 @@ remove_bb (basic_block bb)
>                      into the right OMP region.  */

The comment above says

                  /* During cfg pass make sure to put orphaned labels
                     into the right OMP region.  */

and the full guard is

              if ((unsigned) bb->index < bb_to_omp_idx.length ()
                  && ((unsigned) new_bb->index >= bb_to_omp_idx.length ()
                      || (bb_to_omp_idx[bb->index]
                          != bb_to_omp_idx[new_bb->index])))

The <em>right</em> OMP region suggests something wrt correctness
(though the original choice of bb->prev_bb doesn't put the initial new_bb
in an semantic relation to the original block).

So I wonder what the code was for an whether we still need it.
The ENTRY_BLOCK successor can be in some OMP region I guess
but we're still falling back to a "mismatched" OMP region in case
it was.  Then we could also insert on the ENTRY_BLOCK single successor
edge...

That said, the patch doesn't look incorrect - it just tweaks heuristics - but
the OMP code looks odd.

Richard.

>                   unsigned int i;
>                   int idx;
> -                 new_bb = NULL;
>                   FOR_EACH_VEC_ELT (bb_to_omp_idx, i, idx)
>                     if (i >= NUM_FIXED_BLOCKS
>                         && idx == bb_to_omp_idx[bb->index]
> @@ -2325,11 +2322,6 @@ remove_bb (basic_block bb)
>                         new_bb = BASIC_BLOCK_FOR_FN (cfun, i);
>                         break;
>                       }
> -                 if (new_bb == NULL)
> -                   {
> -                     new_bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> -                     gcc_assert (new_bb != bb);
> -                   }
>                 }
>               new_gsi = gsi_after_labels (new_bb);
>               gsi_remove (&i, false);
>
> --
> 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>