[PATCH 5/6] make get_domminated_by_region return a auto_vec
Trevor Saunders
tbsaunde@tbsaunde.org
Thu Jun 17 08:23:18 GMT 2021
On Thu, Jun 17, 2021 at 08:03:53AM +0200, Richard Biener wrote:
> On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor <msebor@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 <tbsaunde@tbsaunde.org> wrote:
> > >>>
> > >>> This makes it clear the caller owns the vector, and ensures it is cleaned up.
> > >>>
> > >>> Signed-off-by: Trevor Saunders <tbsaunde@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.
For C its certainly true that any struct can be coppied with memcpy or
plain assignment, but that doesn't mean it is expected to work, just
look at vec pre c++, there was a copy macro to copy one. I don't
believe there is a function to copy a hashtab hashtable, presumably
because it was never needed, but here too we see a C datastructure that
can simply be memcpy'd. Its rather narrow to only look at how C and C++
handle things, so lets look at some other languages that have learned
from the previous 30 years. IN no particular order first looking at
python https://www.afternerd.com/blog/python-copy-list/ seems like a
reasonable summary, and we see assignment is just copying a reference to
a list and you call .copy() to dupplicate the list. I believe java is
essentially the same as python perhaps with slightly different names for
methods. I'm not an expert but
https://flaviocopes.com/go-copying-structs/ suggests go is the same as C
except a little better since it has a GC so you don't have to refcount
yourself. As for rust I may have linked it already, but sorry I will
again, because I believe it makes a lot of good points,
https://www.oreilly.com/library/view/programming-rust/9781491927274/ch04.html
assignment moves variables, and you call a function to make a deeper
copy. Its also worth noting the point that rust is enforcing good C++
practice, one of which I'd say is that you move or lend values whenever
possible, rather than making coppies, certainly when copy constructors
where the only thing available making them safe was the only option, but
fortunately today's C++ is very different from the c++ of the 90's, and
we shouldn't keep the same ideas of what's the best practice either.
> > 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.)
Well, copying a mutex can have definable semantics, but I doubt anyone
actually wants copyable mutexs. As for files and streams its easy to
copy files, and streams may be easy depending, and as building blocks
one can certainly argue by your logic they should be copyable. Or we
can take Richi's example, I would think by your logic an important
building block like unique_ptr should support copying by making a copy
of the referenced object, that would preserve the 1:1 relationship
between unique_ptr objects and the objects they own. Further it would
allow me to easily copy vector<unique_ptr<T>> or unordered_map<int,
unique_ptr<T>>. I suspect you would not support making unique_ptr
copyable, so I think the line of what should be copy constructable is
less clear than your making it out to be here, certainly that's a rather
extreme case, but I think under the framework you propose copyable
unique_ptr is defensable.
> > 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
Sorry if I didn't make myself clear, its not so much suprise as a couple
things.
- its unnecessarily easy to make coppies when move would do.
- especially for people not well versed in c++ its hard to tell at a
glance without context if something is a move or a copy. "assignments
are moves, the only way to copy is with copy ()" is a one sentence
rule that can be evaluated looking at the lines in a patch without
checking types or the like. To distinguish a copy and a move you have
to check the type of the RHS is it an r-value reference? is move()
called? is it a return from a function? and probably more things I'm
forgetting at the moment. That's not suprise, its about making it
code easy to read, which is an important part of a good API.
> > 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.
No, they are reasons to ban certain practices, but in some cases while
its a pattern that should be viewed with suspicion its also too useful
to completely ban, nested loops certainly count here. On the other hand
we have banned gets() on the grounds its basically impossible to use
correctly, and provides little value not available other ways. We've
also more or less stopped using varargs functions other than printf or
K&R style function declarations, because they are dangerous and there
are better options.
thanks
Trev
> > 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
> > >>>
> >
More information about the Gcc-patches
mailing list