On 6/21/21 1:15 AM, Richard Biener wrote:
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.htmlWhich 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.
The change disables things that were previously possible (but undefined). It isn't relied on by anything. The change is also incomplete: it disables copying and assignment for the auto_vec<T, 0> specialization but not for the primary template. Neither is safe to copy or assign. I could still submit a patch to fix that and make all auto_vec specializations safely copyable but why should I bother? You have made it abundantly clear that you don't support it and several others have taken your position despite all the problems I have repeatedly pointed out.
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?
Converting an auto_vec object to a vec slices off its data members.
The auto_vec<T, 0> specialization has no data members so that's not
a bug in and of itself, but auto_vec<T, N> does have data members
so that would be a bug. The risk is not just passing it to
functions by value but also returning it. That risk was made
worse by the addition of the move ctor.
Even though it's not strictly a bug, passing an auto_vec<T, 0> to
a function that takes a vec could be surprising if the function
modifies the argument.
Martin
PS Looking at the auto_vec change (and the rest of the definition)
more closely, I note a couple of other questionable things.
The move assignment from vec (and the copy you added) is unsafe(*),
and the test for self-assignment in the move assignment operator
is not needed because an object cannot be moved to itself.
[*] E.g., it allows the following which crashes with a double free
error:
vec<int>
foo (vec<int> v)
{
v.safe_push (2);
v.safe_push (3);
v.safe_push (4);
return v;
}
void bar (auto_vec<int> v) { }
void foobar ()
{
auto_vec<int> v;
v.safe_push (1);
bar (foo (v));
}
My point is not to pick on these other changes per se (I probably
wouldn't have noticed the problems if it wasn't for this discussion)
but to highlight that diverging from best practices tends to have
consequences beyond those we can at fist appreciate, especially if
we're not perfectly comfortable with the rules we're playing with.
Richard.MartinRichard.MartinRichard.MartinThanks, RichardThanks, 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
