On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor <mse...@gmail.com> wrote: > > On 6/18/21 4:38 AM, Richard Biener wrote: > > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor <mse...@gmail.com> wrote: > >> > >> On 6/17/21 12:03 AM, Richard Biener wrote: > >>> 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. > >> > >> 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 > >>>>>>> > >>>> > >> >