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

[Martin Sebor via Gcc-patches]

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

(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