[PATCH 5/6] make get_domminated_by_region return a auto_vec
Richard Biener
richard.guenther@gmail.com
Mon Jun 21 07:15:54 GMT 2021
On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 6/18/21 4:38 AM, Richard Biener wrote:
> > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 6/17/21 12:03 AM, 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.
> >>>>
> >>>> 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.
> >>
> >> Yes, as we discussed in the review below, vec is not a good model
> >> because (as you note again above) it's constrained by its legacy
> >> uses. The best I think we can do for it is to make it safer to
> >> use.
> >> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html
> >
> > Which is what Trevors patches do by simply disallowing things
> > that do not work at the moment.
> >
> >> (Smart pointers don't rule out copying. A std::unique_ptr does
> >> and std::shared_ptr doesn't. But vec and especially auto_vec
> >> are designed to be containers, not "unique pointers" so any
> >> relationship there is purely superficial and a distraction.)
> >>
> >> That auto_vec and vec share a name and an is-a relationship is
> >> incidental, an implementation detail leaked into the API. A better
> >> name than vector is hard to come up with, but the public inheritance
> >> is a design flaw, a bug waiting to be introduced due to the conversion
> >> and the assumptions the base vec makes about POD-ness and shallow
> >> copying. Hindsight is always 20/20 but past mistakes should not
> >> dictate the design of a general purpose vector-like container in
> >> GCC.
> >
> > That auto_vec<> "decays" to vec<> was on purpose design.
> >
> > By-value passing of vec<> is also on purpose to avoid an extra
> > pointer indirection on each access.
>
> I think you may have misunderstood what I mean by is-a relationship.
> It's fine to convert an auto_vec to another interface. The danger
> is in allowing that to happen implicitly because that tends to let
> it happen even when it's not intended. The usual way to avoid
> that risk is to provide a conversion function, like
> auto_vec::to_vec(). This is also why standard classes like
> std::vector or std::string don't allow such implicit conversions
> and instead provide member functions (see for example Stroustrup:
> The C++ Programming Language). So a safer auto_vec class would
> not be publicly derived from vec but instead use the has-a design
> (there are also ways to keep the derivation by deriving both from
> from a limited, safe, interface, that each would extend as
> appropriate).
>
> To the point of by passing vec by value while allowing functions
> to modify the argument: C and C++ have by-value semantics. Every
> C and C++ programmer knows and expect that. Designing interfaces
> that break this assumption is perverse, a sure recipe for bugs.
> If you're concerned about intuitive semantics and surprises you
> should want to avoid that.
>
> >
> >> I fully support fixing or at least mitigating the problems with
> >> the vec base class (unsafe copying, pass-by-value etc.). As I
> >> mentioned, I already started working on this cleanup. I also
> >> have no objection to introducing a non-copyable form of a vector
> >> template (I offered one in my patch), or even to making auto_vec
> >> non-copyable provided a copyable and assignable one is introduced
> >> at the same time, under some other name.
> >
> > Why at the same time? I'm still not convinced we need another
> > vector type here. Yes, auto_vec<auto_vec<..> > would be convenient,
> > but then auto_vec<> doesn't bother to call the DTOR on its elements
> > either (it's actually vec<> again here). So auto_vec<> is _not_
> > a fancier C++ vec<>, it's still just vec<> but with RAII for the container
> > itself.
>
> I don't follow what you're saying. Either you agree that making
> auto_vec suitable as its own element would be useful. If you do,
> it needs to be safely copyable and assignable.
>
> The basic design principle of modern C++ containers is they store
> their elements by value and make no further assumptions. This means
> that an int element is treated the same as int* element as a vec<int>
> element: they're copied (or moved) by their ctors on insertion,
> assigned when being replaced, and destroyed on removal. Containers
> themselves don't, as a rule, manage the resources owned by
> the elements (like auto_delete_vec does). The elements are
> responsible for doing that, which is why they need to be safely
> copyable and assignable. vec meets these requirements because
> it doesn't manage a resource (it's not a container). Its memory
> needs to be managed some other way. auto_vec doesn't. It is
> designed to be a container but it's broken. It won't become one
> by deleting its copy ctor and assignment.
>
> >
> >> Having said that, and although I don't mind the cleanup being taken
> >> off my plate, I would have expected the courtesy of at least a heads
> >> up first. I do find it disrespectful for someone else involved in
> >> the review of my work to at the same time submit a patch of their
> >> own that goes in the opposite direction, and for you to unilaterally
> >> approve it while the other review hasn't concluded yet.
> >
> > Because the changes do not change anything as far as I understand.
> > They make more use of auto_vec<> ownership similar to when
> > I added the move ctor and adjusted a single loop API. At the same
> > time it completes the move stuff and plugs some holes.
>
> The vast majority of Trevor's changes are improvements and I apprciate
> them. But the change to auto_vec goes against best C++ practices and
> in the opposite direction of what I have been arguing for and what
> I submitted a patch for in April. The patch is still under discussion
> that both you and Trevor, as well as Jason, have been participating in.
> We have not concluded that discussion and it's in bad form to simply
> disregard that and proceed in a different direction. My understanding
> from it so far is that
>
> a) you're not opposed to adding the copy ctor:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568827.html
> b) Jason advises to prevent implicit by-value conversion from auto_vec
> to vec
> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571628.html
> c) I said I was working on it (and more, some of it likely now
> obviated by Trevor's changes):
> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html
>
> So would I ask you both to respect that and refrain from approving
> and committing this change (i.e., leave auto_vec as is for now)
> until I've had time to finish my work.
>
> But checking the latest sources I see Trevor already committed
> the change despite this. That's in very poor form, and quite
> disrespectful of both of you.
The change was needed to make the useful portions work as far
as I understood Trevor. There's also nothing that prevents you
from resolving the conflicts and continue with your improvements.
But maybe I'm misunderstanding C++ too much :/
Well, I guess b) from above means auto_vec<> passing to
vec<> taking functions will need changes?
Richard.
> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>>
> >>> 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