Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec

[Martin Sebor <msebor@gmail.com>]

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).

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
>>>