From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32f.google.com (mail-ot1-x32f.google.com [IPv6:2607:f8b0:4864:20::32f]) by sourceware.org (Postfix) with ESMTPS id 1F9653855019 for ; Wed, 16 Jun 2021 16:01:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1F9653855019 Received: by mail-ot1-x32f.google.com with SMTP id 102-20020a9d0eef0000b02903fccc5b733fso2988516otj.4 for ; Wed, 16 Jun 2021 09:01:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=9RPF0PsgrUN8NJYmVM9lvHQRtvWR1Bx9OMwH+LokeGk=; b=TygBbvP4eBxcxuUTH+mL0k+ASrDLdn1cSoPZGeLM6WdNauvJ2ppb3FRyFSf/Gh0/s3 beIHcI5X7CpzW5WmUTvPYSUpjmfaLhmUGmjXQur9qBPglYP5p3eOTCWuoqPCIAab9dY6 +W93U7r0jsD9k2JOuJKpdy4yvz2UWJrgVOpw+qXhMvFKhaoodOksVPLPQsYjMZ3/Jbuu OzH+oAIEWeJV9nM9sdJJmv6b44oVtN8K67wqLmAJDr6lu+BUYPWpWNnmtY3+ILLx18gw onCpoZhR0xUgBKC1yjH4kXPoLexl475bmLTAKE1CmcCj1iH09mDrliQQeEGWAxadAI9H yy4g== X-Gm-Message-State: AOAM5335zLRHSSl5uJ3BNQ77FK3Z5GX1ve7JX9LcCpQ8LhsY9vNdQ2M4 e0mrD+JFlaR5KNak3HC39S8= X-Google-Smtp-Source: ABdhPJx8tM2nQzGmBhcul8YsWNeQ0pC3tiMfrwVvhTSR+SGq35TU+Jiu+IVQckfZKb+HL/m9wczluQ== X-Received: by 2002:a05:6830:181:: with SMTP id q1mr523283ota.360.1623859273378; Wed, 16 Jun 2021 09:01:13 -0700 (PDT) Received: from [192.168.0.41] (97-118-105-195.hlrn.qwest.net. [97.118.105.195]) by smtp.gmail.com with ESMTPSA id n11sm597514otq.63.2021.06.16.09.01.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Jun 2021 09:01:13 -0700 (PDT) Subject: Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec To: Richard Biener via Gcc-patches , Trevor Saunders , Richard Biener , richard.sandiford@arm.com References: <20210615055922.27205-1-tbsaunde@tbsaunde.org> <20210615055922.27205-5-tbsaunde@tbsaunde.org> From: Martin Sebor Message-ID: <9a54c5e1-2d41-f499-e176-84913b810b67@gmail.com> Date: Wed, 16 Jun 2021 10:01:12 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jun 2021 16:01:19 -0000 On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > Richard Biener via Gcc-patches writes: >> On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: >>> >>> This makes it clear the caller owns the vector, and ensures it is cleaned up. >>> >>> Signed-off-by: Trevor Saunders >>> >>> 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 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. >>> * 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 >>> +auto_vec >>> get_dominated_by_region (enum cdi_direction dir, basic_block *region, >>> unsigned n_region) >>> { >>> unsigned i; >>> basic_block dom; >>> - vec doms = vNULL; >>> + auto_vec 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 get_dominated_by (enum cdi_direction, basic_block); >>> -extern vec get_dominated_by_region (enum cdi_direction, >>> +extern auto_vec get_dominated_by_region (enum cdi_direction, >>> basic_block *, >>> unsigned); >>> extern vec 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 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 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 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 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 bbs, dom_bbs; >>> + vec 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 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 >>>