From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by sourceware.org (Postfix) with ESMTPS id 98B713858D37 for ; Thu, 14 Jul 2022 20:12:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 98B713858D37 Received: from [10.10.3.121] (unknown [10.10.3.121]) by mail.ispras.ru (Postfix) with ESMTPS id 2082440D403D; Thu, 14 Jul 2022 20:12:27 +0000 (UTC) Date: Thu, 14 Jul 2022 23:12:27 +0300 (MSK) From: Alexander Monakov To: Richard Biener cc: GCC Patches Subject: Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls In-Reply-To: Message-ID: References: <20220114182047.6270-3-amonakov@ispras.ru> <84dba16-ab1a-f362-48-8f42ebc14d2b@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-8.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Thu, 14 Jul 2022 20:12:35 -0000 On Thu, 14 Jul 2022, Richard Biener wrote: > Indeed. Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right". > > When copying a block we do not copy labels so any "jumps" remain to the original > block and thus we are indeed able to isolate normal control flow. Given that > returns_twice functions _do_ seem to be special, and also special as to how > we handle other abnormal receivers in duplicate_block. We do? Sorry, I don't see what you mean here, can you point me to specific lines? > So it might indeed make sense to special-case them in can_duplicate_block_p > ... (sorry for going back-and-forth ...) > > Note that I think this detail of duplicate_block (the function) and the hook > needs to be better documented (the semantics on incoming edges, not duplicating > labels used for incoming control flow). > > Can you see as to how to adjust the RTL side for this? It looks like at least > some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder > if in rtl_verify_flow_info[_1] (or its callees) we can check that such > calls come > first ... they might not since IIRC we do _not_ preserve abnormal edges when > expanding RTL (there's some existing bug about this and how this breaks some > setjmp tests) (but we try to recompute them?). No, we emit arguments/return value handling before/after a REG_SETJMP call, and yeah, we don't always properly recompute abnormal edges, so improving RTL in this respect seems hopeless. For example, it is easy enough to create a testcase where bb-reordering duplicates a returns_twice call, although it runs so late that perhaps later passes don't care: // gcc -O2 --param=max-grow-copy-bb-insns=100 __attribute__((returns_twice)) int rtwice(int); int g1(int), g2(int); void f(int i) { do { i = i%2 ? g1(i) : g2(i); } while (i = rtwice(i)); } FWIW, I also investigated https://gcc.gnu.org/PR101347 > Sorry about the back-and-forth again ... your original patch looks OK for the > GIMPLE side but can you amend the cfghooks.{cc,h} documentation to > summarize our findings and > the desired semantics of duplicate_block in this respect? Like below? ---8<--- Subject: [PATCH v3] tree-cfg: do not duplicate returns_twice calls A returns_twice call may have associated abnormal edges that correspond to the "second return" from the call. If the call is duplicated, the copies of those edges also need to be abnormal, but e.g. tracer does not enforce that. Just prohibit the (unlikely to be useful) duplication. gcc/ChangeLog: * cfghooks.cc (duplicate_block): Expand comment. * tree-cfg.cc (gimple_can_duplicate_bb_p): Reject blocks with calls that may return twice. --- gcc/cfghooks.cc | 13 ++++++++++--- gcc/tree-cfg.cc | 7 +++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc index e435891fa..c6ac9532c 100644 --- a/gcc/cfghooks.cc +++ b/gcc/cfghooks.cc @@ -1086,9 +1086,16 @@ can_duplicate_block_p (const_basic_block bb) return cfg_hooks->can_duplicate_block_p (bb); } -/* Duplicates basic block BB and redirects edge E to it. Returns the - new basic block. The new basic block is placed after the basic block - AFTER. */ +/* Duplicate basic block BB, place it after AFTER (if non-null) and redirect + edge E to it (if non-null). Return the new basic block. + + If BB contains a returns_twice call, the caller is responsible for recreating + incoming abnormal edges corresponding to the "second return" for the copy. + gimple_can_duplicate_bb_p rejects such blocks, while RTL likes to live + dangerously. + + If BB has incoming abnormal edges for some other reason, their destinations + should be tied to label(s) of the original BB and not the copy. */ basic_block duplicate_block (basic_block bb, edge e, basic_block after, copy_bb_data *id) diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index f846dc2d8..5bcf78198 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -6346,12 +6346,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb) { gimple *g = gsi_stmt (gsi); - /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be + /* Prohibit duplication of returns_twice calls, otherwise associated + abnormal edges also need to be duplicated properly. + An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be duplicated as part of its group, or not at all. The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a group, so the same holds there. */ if (is_gimple_call (g) - && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC) + && (gimple_call_flags (g) & ECF_RETURNS_TWICE + || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC) || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT) || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY) || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY) -- 2.35.1