From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 863BA385381B for ; Tue, 2 Aug 2022 13:29:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 863BA385381B Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 483331FB03; Tue, 2 Aug 2022 13:29:43 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 2A5932C141; Tue, 2 Aug 2022 13:29:43 +0000 (UTC) Date: Tue, 2 Aug 2022 13:29:43 +0000 (UTC) From: Richard Biener To: Aldy Hernandez cc: gcc-patches , "MacLeod, Andrew" , Jeff Law , Jan Hubicka Subject: Re: [PATCH] Properly honor param_max_fsm_thread_path_insns in backwards threader In-Reply-To: Message-ID: References: <04261.122080204410800126@us-mta-529.us.mimecast.lan> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP 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: Tue, 02 Aug 2022 13:29:45 -0000 On Tue, 2 Aug 2022, Aldy Hernandez wrote: > On Tue, Aug 2, 2022 at 1:59 PM Richard Biener wrote: > > > > On Tue, 2 Aug 2022, Aldy Hernandez wrote: > > > > > On Tue, Aug 2, 2022 at 1:45 PM Richard Biener wrote: > > > > > > > > On Tue, 2 Aug 2022, Aldy Hernandez wrote: > > > > > > > > > Unfortunately, this was before my time, so I don't know. > > > > > > > > > > That being said, thanks for tackling these issues that my work > > > > > triggered last release. Much appreciated. > > > > > > > > Ah. But it was your r12-324-g69e5544210e3c0 that did > > > > > > > > - else if (n_insns > 1) > > > > + else if (!m_speed_p && n_insns > 1) > > > > > > > > causing the breakage on the 12 branch. That leads to a simpler > > > > fix I guess. Will re-test and also backport to GCC 12 if successful. > > > > > > Huh. It's been a while, but that looks like a typo. That patch was > > > supposed to be non-behavior changing. > > > > Exactly my thinking so reverting it shouldn't be a reason for > > detailed questions. Now, the contains_hot_bb computation is, > > Sorry for the pain. So - actually the change was probably done on purpose (even if reverting - which I've now already one - caused no testsuite regressions). That's because the whole function is invoked N + 1 times for a path of length N and we definitely want to avoid using the size optimization heuristics when the path is not complete yet. I think the proper way is to do diff --git a/gcc/tree-ssa-threadbackward.cc b/gcc/tree-ssa-threadbackward.cc index ba114e98a41..6979398ef76 100644 --- a/gcc/tree-ssa-threadbackward.cc +++ b/gcc/tree-ssa-threadbackward.cc @@ -767,7 +767,11 @@ back_threader_profitability::profitable_path_p (const vec &m_path, as in PR 78407 this leads to noticeable improvements. */ if (m_speed_p && ((taken_edge && optimize_edge_for_speed_p (taken_edge)) - || contains_hot_bb)) + || contains_hot_bb + /* Avoid using the size heuristics when not doing the final + thread evaluation, we get called for each added BB + to the path. */ + || !taken_edge)) { if (n_insns >= param_max_fsm_thread_path_insns) { thus assume there'll be a hot BB in the future. That said, the very best fix would be to not call this function N + 1 times (I have a patch to call it only N times - yay), but instead factor out parts to be called per BB plus keeping enough state so we can incrementally collect info. There's more "odd" things in the backward threader, of course :/ I'm looking for things applicable to the GCC 12 branch right now so will try the above. Richard.