| Message ID | orim1eucvv.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | move unreachable user labels to entry point | expand |
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>
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
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 >
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.
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);