move unreachable user labels to entry point

move unreachable user labels to entry point

[Alexandre Oliva]

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.  */
 		  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>

Re: move unreachable user labels to entry point

[Richard Biener]

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>

Re: move unreachable user labels to entry point

[Jakub Jelinek]

On Tue, Jul 13, 2021 at 09:56:41AM +0200, Richard Biener wrote:
> 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).

The reason for the OMP code I've added there is to put orphaned labels into
the right OpenMP region, because if they are moved to arbitrary unrelated
locations, when outlining the OMP regions (e.g. parallel, task, or target),
we want the orphaned labels to be in the right function as opposed to some
unrelated one, at least if the orphaned label is referenced in the code.
Because referencing say from offloaded code a label that got moved to the
host function or vice versa simply can't work and similarly causes ICEs
even with parallel/task etc. regions.

But I'm not sure I agree with the intention of the patch, yes, orphaned
labels even without OpenMP are moved to some other place, but the current
code typically keeps it close to where it used to live.  Moving them to
entry will be always worse user experience if people e.g. print the &&label
addresses etc.

	Jakub

Re: move unreachable user labels to entry point

[Richard Biener]

On Tue, Jul 13, 2021 at 10:52 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Jul 13, 2021 at 09:56:41AM +0200, Richard Biener wrote:
> > 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).
>
> The reason for the OMP code I've added there is to put orphaned labels into
> the right OpenMP region, because if they are moved to arbitrary unrelated
> locations, when outlining the OMP regions (e.g. parallel, task, or target),
> we want the orphaned labels to be in the right function as opposed to some
> unrelated one, at least if the orphaned label is referenced in the code.
> Because referencing say from offloaded code a label that got moved to the
> host function or vice versa simply can't work and similarly causes ICEs
> even with parallel/task etc. regions.
>
> But I'm not sure I agree with the intention of the patch, yes, orphaned
> labels even without OpenMP are moved to some other place, but the current
> code typically keeps it close to where it used to live.

typically, yes, but bb->prev_bb doesn't really have such meaning.  OTOH
it's hard to do better from delete_basic_block, callers might do a better
job but there's currently no way to communicate down sth like an
entry or exit to a removed CFG region where these things could be moved to.

Maybe the GIMPLE CFG hooks could queue to be re-issued stmts somewhere
and callers be responsible for re-emitting them.

>  Moving them to
> entry will be always worse user experience if people e.g. print the &&label
> addresses etc.
>
>         Jakub
>

Re: move unreachable user labels to entry point

[Alexandre Oliva]

On Jul 13, 2021, Richard Biener <richard.guenther@gmail.com> wrote:

> The <em>right</em> OMP region suggests something wrt correctness

Yeah, as Jakub wrote, we have to choose a block that's in the same
region the label belongs to.  The proposed patch doesn't change that, it
just uses the entry block instead of the previous block, if it satisfies
the requirement.

I found it made the logic simpler, slightly more efficient, and more
predictable, but I'm not attached to the change.  Since Jakub objected
to it, let's leave it alone.

-- 
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>