From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id B4C763858D3C for ; Tue, 19 Apr 2022 12:58:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B4C763858D3C Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id DC59021118; Tue, 19 Apr 2022 12:58:26 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id BC84B2C142; Tue, 19 Apr 2022 12:58:26 +0000 (UTC) Date: Tue, 19 Apr 2022 14:58:26 +0200 (CEST) From: Richard Biener To: Segher Boessenkool cc: gcc-patches@gcc.gnu.org, rth@redhat.com, ebotcazou@adacore.com, jason@redhat.com Subject: Re: [PATCH][v2] rtl-optimization/105231 - distribute_notes and REG_EH_REGION In-Reply-To: <20220419122907.GG25951@gate.crashing.org> Message-ID: <16sn7s3n-s5s6-331r-4prn-q26n712s4sss@fhfr.qr> References: <20220419110209.A4C1F139BE@imap2.suse-dmz.suse.de> <20220419122907.GG25951@gate.crashing.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Tue, 19 Apr 2022 12:58:29 -0000 On Tue, 19 Apr 2022, Segher Boessenkool wrote: > Hi! > > So the assert last week was for a landing pad <= 0? < or =? The assert was for any landing pad which obviously failed - the testsuite fails were all for MUST_NOT_THROW (negative) regions which do not end basic-blocks. > On Tue, Apr 19, 2022 at 01:02:09PM +0200, Richard Biener wrote: > > The following mitigates a problem in combine distribute_notes which > > places an original REG_EH_REGION based on only may_trap_p which is > > good to test whether a non-call insn can possibly throw but not if > > actually it does or we care. That's something we decided at RTL > > expansion time where we possibly still know the insn evaluates > > to a constant. > > > > In fact, the REG_EH_REGION can only come from the original i3 and > > an assert is added to that effect. That means we only need to > > retain the note on i3 or, if that cannot trap, drop it but we > > should never move it to i2. If splitting of i3 ever becomes a > > problem here the insn split should be rejected instead. > > > > We are also considering all REG_EH_REGION equal, including > > must-not-throw and nothrow kinds but when those are not from i3 > > we have no good idea what should happen to them, so the following > > simply drops them. > > And that always is safe? Why do we have REG_EH_REGION for those cases > at all, then? It's the only "safe" thing to do at distribute_notes time I think. If it is not "safe" (it might lose must-not-throw EH events, or lose optimization when dropping nothrow) we probably have to reject the combination earlier. As I understand combining to multiple insns always happens via a split (so we combine up to three insns into one and then might end up splitting that into at most two insns). The only case we could in theory special-case is when _all_ original insns combined have the exact same REG_EH_REGION (all have it with the same landing pad number) or some have none but i3 at least has one. Then we should be able to distribute the note to all possibly two result insns. But I can't see that distribute_note has this info readily available (that there not exist conflicting REG_EH_REGIONs, like MNT on the original i2 and a > 0 one on i3). > > + { > > + int lp_nr = INTVAL (XEXP (note, 0)); > > + /* A REG_EH_REGION note transfering control can only ever come > > + from i3. */ > > + gcc_assert (lp_nr <= 0 || from_insn == i3); > > if (lp_nr > 0) > gcc_assert (from_insn == i3); > is less obfuscated ;-) I find that less obvious, but you can have it your way if you like. > > + /* For nothrow (lp_nr == 0 or lp_nr == INT_MIN) and > > + for insns in a MUST_NOT_THROW region (lp_nr < 0) > > + it's difficult to decide what to do for notes > > + coming from an insn that is not i3. Just drop > > + those. */ > > That sounds like we do not know what is correct to do, so just sweep it > under the carpet and hope it works out. "Just drop those, that is > always safe"? (Is it always safe?) If it is not safe we should have rejected the combination. I fully expect that we'd need to have a piece during analysis constraining what cases we feed into here to be really "safe". I'm really not familiar with combine so I know nothing of the constraints it has (like is only i3 ever going to be a CALL_INSN_P?) > Okay for trunk with maybe a word or two more there. Thanks! I'll see if there are more comments before pushing. Thanks, Richard.