From: Richard Biener <richard.guenther@gmail.com>
To: Martin Sebor <msebor@gmail.com>
Cc: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org>,
Trevor Saunders <tbsaunde@tbsaunde.org>,
Richard Sandiford <richard.sandiford@arm.com>
Subject: Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec
Date: Fri, 18 Jun 2021 12:38:09 +0200 [thread overview]
Message-ID: <CAFiYyc3q-V1DBjOSFRVZkdW2vG93fyCYUAeS1KAZGjGX4C9yzQ@mail.gmail.com> (raw)
In-Reply-To: <125764da-111e-7e44-b2ba-8389644423e8@gmail.com>
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 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.
> 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.
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
> >>>>>
> >>
>
next prev parent reply other threads:[~2021-06-18 10:38 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-15 5:59 [PATCH 1/6] auto_vec copy/move improvements Trevor Saunders
2021-06-15 5:59 ` [PATCH 2/6] return auto_vec from cgraph_node::collect_callers Trevor Saunders
2021-06-15 6:45 ` Richard Biener
2021-06-15 5:59 ` [PATCH 3/6] return auto_vec from get_loop_hot_path Trevor Saunders
2021-06-15 6:45 ` Richard Biener
2021-06-17 13:48 ` Christophe Lyon
2021-06-17 14:41 ` Trevor Saunders
2021-06-17 18:34 ` Christophe Lyon
2021-06-15 5:59 ` [PATCH 4/6] return auto_vec from get_dominated_by Trevor Saunders
2021-06-15 6:46 ` Richard Biener
2021-06-15 11:18 ` Bernhard Reutner-Fischer
2021-06-16 3:09 ` Trevor Saunders
2021-06-16 5:45 ` Bernhard Reutner-Fischer
2021-06-17 6:56 ` Trevor Saunders
2021-06-15 5:59 ` [PATCH 5/6] make get_domminated_by_region return a auto_vec Trevor Saunders
2021-06-15 6:49 ` Richard Biener
2021-06-16 12:46 ` Richard Sandiford
2021-06-16 16:01 ` Martin Sebor
2021-06-17 6:03 ` Richard Biener
2021-06-17 8:23 ` Trevor Saunders
2021-06-17 14:43 ` Martin Sebor
2021-06-18 10:38 ` Richard Biener [this message]
2021-06-18 10:53 ` Jakub Jelinek
2021-06-18 11:03 ` Jonathan Wakely
2021-06-18 11:04 ` Jonathan Wakely
2021-06-18 16:03 ` Martin Sebor
2021-06-21 7:15 ` Richard Biener
2021-06-22 20:01 ` Martin Sebor
2021-06-23 5:23 ` Trevor Saunders
2021-06-23 7:43 ` Richard Biener
2021-06-23 10:22 ` Richard Sandiford
2021-06-24 9:20 ` Richard Biener
2021-06-24 16:28 ` Richard Sandiford
2021-06-25 8:29 ` Richard Biener
2021-06-23 22:56 ` Martin Sebor
2021-06-24 9:27 ` Richard Biener
2021-06-24 15:01 ` Martin Sebor
2021-06-23 23:43 ` Martin Sebor
2021-06-28 7:01 ` Trevor Saunders
2021-06-15 5:59 ` [PATCH 6/6] return auto_vec from more dominance functions Trevor Saunders
2021-06-15 6:50 ` Richard Biener
2021-06-15 6:42 ` [PATCH 1/6] auto_vec copy/move improvements Richard Biener
2021-06-15 7:04 ` Trevor Saunders
2021-06-15 7:11 ` Richard Biener
2021-06-15 7:57 ` Trevor Saunders
2021-06-15 9:36 ` Richard Biener
2021-06-16 3:17 ` [PATCH, V2] " Trevor Saunders
2021-06-16 10:13 ` Richard Biener
2021-06-16 17:01 ` Martin Sebor
2021-06-15 16:18 ` [PATCH 1/6] " Martin Sebor
2021-06-16 3:31 ` Trevor Saunders
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc3q-V1DBjOSFRVZkdW2vG93fyCYUAeS1KAZGjGX4C9yzQ@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@gmail.com \
--cc=richard.sandiford@arm.com \
--cc=tbsaunde@tbsaunde.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).