[PATCH 5/6] make get_domminated_by_region return a auto_vec
Martin Sebor
msebor@gmail.com
Thu Jun 17 14:43:54 GMT 2021
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
(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.
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.
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.
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