On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote:
> > Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> >> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders <tbsau...@tbsaunde.org>
> >> wrote:
> >>>
> >>> This makes it clear the caller owns the vector, and ensures it is cleaned
> >>> up.
> >>>
> >>> Signed-off-by: Trevor Saunders <tbsau...@tbsaunde.org>
> >>>
> >>> bootstrapped and regtested on x86_64-linux-gnu, ok?
> >>
> >> OK.
> >>
> >> Btw, are "standard API" returns places we can use 'auto'? That would avoid
> >> excessive indent for
> >>
> >> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> >> - bbs.address (),
> >> - bbs.length ());
> >> + auto_vec<basic_block> dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> >> + bbs.address (),
> >> + bbs.length ());
> >>
> >> and just uses
> >>
> >> auto dom_bbs = get_dominated_by_region (...
> >>
> >> Not asking you to do this, just a question for the audience.
> >
> > Personally I think this would be surprising for something that doesn't
> > have copy semantics. (Not that I'm trying to reopen that debate here :-)
> > FWIW, I agree not having copy semantics is probably the most practical
> > way forward for now.)
>
> But you did open the door for me to reiterate my strong disagreement
> with that. The best C++ practice going back to the early 1990's is
> to make types safely copyable and assignable. It is the default for
> all types, in both C++ and C, and so natural and expected.
>
> Preventing copying is appropriate in special and rare circumstances
> (e.g, a mutex may not be copyable, or a file or iostream object may
> not be because they represent a unique physical resource.)
>
> In the absence of such special circumstances preventing copying is
> unexpected, and in the case of an essential building block such as
> a container, makes the type difficult to use.
>
> The only argument for disabling copying that has been given is
> that it could be surprising(*). But because all types are copyable
> by default the "surprise" is usually when one can't be.
>
> I think Richi's "surprising" has to do with the fact that it lets
> one inadvertently copy a large amount of data, thus leading to
> an inefficiency. But by analogy, there are infinitely many ways
> to end up with inefficient code (e.g., deep recursion, or heap
> allocation in a loop), and they are not a reason to ban the coding
> constructs that might lead to it.
>
> IIUC, Jason's comment about surprising effects was about implicit
> conversion from auto_vec to vec. I share that concern, and agree
> that it should be addressed by preventing the conversion (as Jason
> suggested).
But fact is that how vec<> and auto_vec<> are used today in GCC
do not favor that. In fact your proposed vec<> would be quite radically
different (and IMHO vec<> and auto_vec<> should be unified then to
form your proposed new container). auto_vec<> at the moment simply
maintains ownership like a smart pointer - which is _also_ not copyable.
Richard.
> Martin
>
> >
> > Thanks,
> > Richard
> >
> >> Thanks,
> >> Richard.
> >>
> >>> gcc/ChangeLog:
> >>>
> >>> * dominance.c (get_dominated_by_region): Return
> >>> auto_vec<basic_block>.
> >>> * dominance.h (get_dominated_by_region): Likewise.
> >>> * tree-cfg.c (gimple_duplicate_sese_region): Adjust.
> >>> (gimple_duplicate_sese_tail): Likewise.
> >>> (move_sese_region_to_fn): Likewise.
> >>> ---
> >>> gcc/dominance.c | 4 ++--
> >>> gcc/dominance.h | 2 +-
> >>> gcc/tree-cfg.c | 18 +++++++-----------
> >>> 3 files changed, 10 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/gcc/dominance.c b/gcc/dominance.c
> >>> index 0e464cb7282..4943102ff1d 100644
> >>> --- a/gcc/dominance.c
> >>> +++ b/gcc/dominance.c
> >>> @@ -906,13 +906,13 @@ get_dominated_by (enum cdi_direction dir,
> >>> basic_block bb)
> >>> direction DIR) by some block between N_REGION ones stored in REGION,
> >>> except for blocks in the REGION itself. */
> >>>
> >>> -vec<basic_block>
> >>> +auto_vec<basic_block>
> >>> get_dominated_by_region (enum cdi_direction dir, basic_block *region,
> >>> unsigned n_region)
> >>> {
> >>> unsigned i;
> >>> basic_block dom;
> >>> - vec<basic_block> doms = vNULL;
> >>> + auto_vec<basic_block> doms;
> >>>
> >>> for (i = 0; i < n_region; i++)
> >>> region[i]->flags |= BB_DUPLICATED;
> >>> diff --git a/gcc/dominance.h b/gcc/dominance.h
> >>> index 515a369aacf..c74ad297c6a 100644
> >>> --- a/gcc/dominance.h
> >>> +++ b/gcc/dominance.h
> >>> @@ -47,7 +47,7 @@ extern basic_block get_immediate_dominator (enum
> >>> cdi_direction, basic_block);
> >>> extern void set_immediate_dominator (enum cdi_direction, basic_block,
> >>> basic_block);
> >>> extern auto_vec<basic_block> get_dominated_by (enum cdi_direction,
> >>> basic_block);
> >>> -extern vec<basic_block> get_dominated_by_region (enum cdi_direction,
> >>> +extern auto_vec<basic_block> get_dominated_by_region (enum cdi_direction,
> >>> basic_block *,
> >>> unsigned);
> >>> extern vec<basic_block> get_dominated_to_depth (enum cdi_direction,
> >>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> >>> index 6bdd1a561fd..c9403deed19 100644
> >>> --- a/gcc/tree-cfg.c
> >>> +++ b/gcc/tree-cfg.c
> >>> @@ -6495,7 +6495,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
> >>> bool free_region_copy = false, copying_header = false;
> >>> class loop *loop = entry->dest->loop_father;
> >>> edge exit_copy;
> >>> - vec<basic_block> doms = vNULL;
> >>> edge redirected;
> >>> profile_count total_count = profile_count::uninitialized ();
> >>> profile_count entry_count = profile_count::uninitialized ();
> >>> @@ -6549,9 +6548,9 @@ gimple_duplicate_sese_region (edge entry, edge exit,
> >>>
> >>> /* Record blocks outside the region that are dominated by something
> >>> inside. */
> >>> + auto_vec<basic_block> doms;
> >>> if (update_dominance)
> >>> {
> >>> - doms.create (0);
> >>> doms = get_dominated_by_region (CDI_DOMINATORS, region, n_region);
> >>> }
> >>>
> >>> @@ -6596,7 +6595,6 @@ gimple_duplicate_sese_region (edge entry, edge exit,
> >>> set_immediate_dominator (CDI_DOMINATORS, entry->dest, entry->src);
> >>> doms.safe_push (get_bb_original (entry->dest));
> >>> iterate_fix_dominators (CDI_DOMINATORS, doms, false);
> >>> - doms.release ();
> >>> }
> >>>
> >>> /* Add the other PHI node arguments. */
> >>> @@ -6662,7 +6660,6 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
> >>> class loop *loop = exit->dest->loop_father;
> >>> class loop *orig_loop = entry->dest->loop_father;
> >>> basic_block switch_bb, entry_bb, nentry_bb;
> >>> - vec<basic_block> doms;
> >>> profile_count total_count = profile_count::uninitialized (),
> >>> exit_count = profile_count::uninitialized ();
> >>> edge exits[2], nexits[2], e;
> >>> @@ -6705,7 +6702,8 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
> >>>
> >>> /* Record blocks outside the region that are dominated by something
> >>> inside. */
> >>> - doms = get_dominated_by_region (CDI_DOMINATORS, region, n_region);
> >>> + auto_vec<basic_block> doms = get_dominated_by_region (CDI_DOMINATORS,
> >>> region,
> >>> + n_region);
> >>>
> >>> total_count = exit->src->count;
> >>> exit_count = exit->count ();
> >>> @@ -6785,7 +6783,6 @@ gimple_duplicate_sese_tail (edge entry, edge exit,
> >>> /* Anything that is outside of the region, but was dominated by
> >>> something
> >>> inside needs to update dominance info. */
> >>> iterate_fix_dominators (CDI_DOMINATORS, doms, false);
> >>> - doms.release ();
> >>> /* Update the SSA web. */
> >>> update_ssa (TODO_update_ssa);
> >>>
> >>> @@ -7567,7 +7564,7 @@ basic_block
> >>> move_sese_region_to_fn (struct function *dest_cfun, basic_block
> >>> entry_bb,
> >>> basic_block exit_bb, tree orig_block)
> >>> {
> >>> - vec<basic_block> bbs, dom_bbs;
> >>> + vec<basic_block> bbs;
> >>> basic_block dom_entry = get_immediate_dominator (CDI_DOMINATORS,
> >>> entry_bb);
> >>> basic_block after, bb, *entry_pred, *exit_succ, abb;
> >>> struct function *saved_cfun = cfun;
> >>> @@ -7599,9 +7596,9 @@ move_sese_region_to_fn (struct function *dest_cfun,
> >>> basic_block entry_bb,
> >>>
> >>> /* The blocks that used to be dominated by something in BBS will now
> >>> be
> >>> dominated by the new block. */
> >>> - dom_bbs = get_dominated_by_region (CDI_DOMINATORS,
> >>> - bbs.address (),
> >>> - bbs.length ());
> >>> + auto_vec<basic_block> dom_bbs = get_dominated_by_region
> >>> (CDI_DOMINATORS,
> >>> + bbs.address (),
> >>> + bbs.length ());
> >>>
> >>> /* Detach ENTRY_BB and EXIT_BB from CFUN->CFG. We need to remember
> >>> the predecessor edges to ENTRY_BB and the successor edges to
> >>> @@ -7937,7 +7934,6 @@ move_sese_region_to_fn (struct function *dest_cfun,
> >>> basic_block entry_bb,
> >>> set_immediate_dominator (CDI_DOMINATORS, bb, dom_entry);
> >>> FOR_EACH_VEC_ELT (dom_bbs, i, abb)
> >>> set_immediate_dominator (CDI_DOMINATORS, abb, bb);
> >>> - dom_bbs.release ();
> >>>
> >>> if (exit_bb)
> >>> {
> >>> --
> >>> 2.20.1
> >>>
>
