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

[Martin Sebor via Gcc-patches]

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

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