On Tue, Jul 13, 2021 at 5:11 AM Alexandre Oliva <ol...@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>
