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 CECDE3856DE5 for ; Wed, 13 Jul 2022 14:57:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CECDE3856DE5 Received: from [10.10.3.121] (unknown [10.10.3.121]) by mail.ispras.ru (Postfix) with ESMTPS id B5CC440D4004; Wed, 13 Jul 2022 14:57:11 +0000 (UTC) Date: Wed, 13 Jul 2022 17:57:11 +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: <84dba16-ab1a-f362-48-8f42ebc14d2b@ispras.ru> References: <20220114182047.6270-3-amonakov@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: Wed, 13 Jul 2022 14:57:18 -0000 On Wed, 13 Jul 2022, Richard Biener wrote: > > > The thing to check would be incoming abnormal edges in > > > can_duplicate_block_p, not (only) returns twice functions? > > > > Unfortunately not, abnormal edges are also used for computed gotos, which are > > less magic than returns_twice edges and should not block tracer I think. > > I think computed gotos should use regular edges, only non-local goto should > use abnormals... Yeah, afaict it's not documented what "abnormal" is supposed to mean :/ > I suppose asm goto also uses abnormal edges? Heh, no, asm goto appears to use normal edges, but there's an old gap in their specification: can you use them like computed gotos, i.e. can asm-goto jump to a computed target? Or must they be similar to plain gotos where the jump label is redirectable (because it's substitutable in the asm template)? If you take a restrictive interpretation (asm goto may not jump to a computed label) then using regular edges looks fine. > Btw, I don't see how they in general are "less magic". Sure, we have an > explicit receiver (the destination label), but we can only do edge inserts > if we have a single computed goto edge into a block (we can "move" the > label to the block created when splitting the edge). Sure, they are a bit magic, but returns_twice edges are even more magic: their destination looks tied to a label in the IR, but in reality their destination is inside a call that returns twice (hence GCC must be careful not to insert anything between the label and the call, like in patch 1/3). > > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets. > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html > > > > How would you like to proceed here? Is my initial patch ok? > > Hmm, so for returns twice calls duplicate_block correctly copies the call > and redirects the provided incoming edge to it. The API does not > handle adding any further incoming edges - the caller would be responsible > for this. So I still somewhat fail to see the point here. If tracer does not > handle extra incoming edges properly then we need to fix tracer? I think abnormal edges corresponding to computed gotos are fine: we are attempting to create a chain of blocks with no incoming edges in the middle, right? Destinations of computed gotos remain at labels of original blocks. Agreed about correcting this in the tracer. > This also includes non-local goto (we seem to copy non-local labels just > fine - wasn't there a bugreport about this!?). Sorry, no idea about this. > So I think can_duplicate_block_p is the wrong place to fix (the RTL side > would need a similar fix anyhow?) Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is, and instead constrain just the tracer. Alternative patch below: * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls. diff --git a/gcc/tracer.cc b/gcc/tracer.cc index 64517846d..422e2b6a7 100644 --- a/gcc/tracer.cc +++ b/gcc/tracer.cc @@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count) gimple *stmt; int n = 0; + bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb)); + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) { stmt = gsi_stmt (gsi); n += estimate_num_insns (stmt, &eni_size_weights); + if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL + && gimple_call_flags (stmt) & ECF_RETURNS_TWICE) + can_dup = false; } *count = n; - cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb))); + cache_can_duplicate_bb_p (bb, can_dup); } /* Return true if E1 is more frequent than E2. */