From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 5986C3858039 for ; Wed, 2 Feb 2022 09:27:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5986C3858039 Received: by mail-ed1-x52f.google.com with SMTP id b13so40803629edn.0 for ; Wed, 02 Feb 2022 01:27:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PnBradsIxp++Mce/iT80suK/NoVWPlENJhe1ee7C5t4=; b=WeUzOA/ppEXLgEvTNWEz+5QBNDqEMWEtWKl/SqCN5Zp6uF2uARIihYocwdGcCnun+e rLAvMSaD2Jvgn0hgzY/FmiOvnyApDZQuE2PvfRFPp1DuUG5RBKC94vd0U/g5+ymS/Ulc /RaUFd0qlUVrMxYQED+LK1yWEWFBkOEYjSuWB/yc+rfcBkwR6IwgcoqjZR6Dt9Olzym7 SEWIpjhZBat2nWwK9nRXvoukimyyOXgE5WrmaiJ5ZlWjJlJyLfY5JQ5veKjRwDQP+1Yn mCokYuMnwcWGt7SCjrQavcRXritGwMII9d4G1km1FlLrmO5nyNL8aVE6PVf9MQqbDyyP 2PQA== X-Gm-Message-State: AOAM533V8X+jSd7EmifIJBKhuNSxVXebNkzxxoRneeKc2wNWaAzVt8n9 +tdnwqr+qnE6woq36+74TZJrPMjDBJhs18+9cm8= X-Google-Smtp-Source: ABdhPJyLW6jyjKOvAxZph8a5XJoSv+QRGnSvcZmRtvaKSdW0vg+Ip3uag9T58YVPigGaTHX+dwvMsA4a4ELhmDX/Cw8= X-Received: by 2002:a05:6402:2688:: with SMTP id w8mr28986689edd.393.1643794055429; Wed, 02 Feb 2022 01:27:35 -0800 (PST) MIME-Version: 1.0 References: <20220121082901.223286-1-aldyh@redhat.com> In-Reply-To: From: Richard Biener Date: Wed, 2 Feb 2022 10:27:24 +0100 Message-ID: Subject: Re: [PATCH] Reset relations when crossing backedges. To: Aldy Hernandez Cc: GCC patches , Jeff Law , "MacLeod, Andrew" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, 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: Wed, 02 Feb 2022 09:27:38 -0000 On Tue, Feb 1, 2022 at 7:41 PM Aldy Hernandez wrote: > > Ping I didn't quite get Jeffs comment, so I waited (sorry). I've meanwhile added extern bool mark_dfs_back_edges (struct function *); so please make verify_mark_backedges take a struct function * and replace 'cfun' with the explicit argument. + gcc_unreachable (); should be sth like internal_error ("% failed"); OK with those changes. Thanks, Richard. > On Mon, Jan 24, 2022, 20:20 Aldy Hernandez wrote: >> >> On Mon, Jan 24, 2022 at 9:51 AM Richard Biener >> wrote: >> > >> > On Fri, Jan 21, 2022 at 1:12 PM Aldy Hernandez wrote: >> > > >> > > On Fri, Jan 21, 2022 at 11:56 AM Richard Biener >> > > wrote: >> > > > >> > > > On Fri, Jan 21, 2022 at 11:30 AM Aldy Hernandez wrote: >> > > > > >> > > > > On Fri, Jan 21, 2022 at 10:43 AM Richard Biener >> > > > > wrote: >> > > > > > >> > > > > > On Fri, Jan 21, 2022 at 9:30 AM Aldy Hernandez via Gcc-patches >> > > > > > wrote: >> > > > > > > >> > > > > > > As discussed in PR103721, the problem here is that we are crossing a >> > > > > > > backedge and causing us to use relations from a previous iteration of a >> > > > > > > loop. >> > > > > > > >> > > > > > > This handles the testcases in both PR103721 and PR104067 which are variants >> > > > > > > of the same thing. >> > > > > > > >> > > > > > > Tested on x86-64 Linux with the usual regstrap as well as verifying the >> > > > > > > thread count before and after the patch. The number of threads is >> > > > > > > reduced by a miniscule amount. >> > > > > > > >> > > > > > > I assume we need release manager approval at this point? OK for trunk? >> > > > > > >> > > > > > Not for regression fixes. >> > > > > >> > > > > OK, I've pushed it to fix the P1s. We can continue refining the >> > > > > solution in a follow-up patch. >> > > > > >> > > > > > >> > > > > > Btw, I wonder whether you have to treat irreducible regions in the same >> > > > > > way more generally - which edges are marked as backedge there depends >> > > > > > on which edge into the region was visited first. I also wonder how we >> > > > > >> > > > > Jeff, Andrew?? >> > > > > >> > > > > > I also wonder how we guarantee that all users of the resolve mode have backedges marked >> > > > > > properly? Also note that CFG manipulation routines in general do not >> > > > > > keep backedge markings up-to-date so incremental modification and >> > > > > > resolving queries might not mix. >> > > > > > >> > > > > > It's a bit unfortunate that we can't query the CFG on whether backedges >> > > > > > are marked. >> > > > > >> > > > > Ughh. The call to mark_dfs_back_edges is currently in the backward >> > > > > threader. Perhaps we could put it in the path_range_query >> > > > > constructor? That way other users of path_range_query can benefit >> > > > > (loop_ch for instance, though it doesn't use it in a way that crosses >> > > > > backedges so perhaps it's unaffected). Does that sound reasonable? >> > > > >> > > > Hmm, I'd rather keep the burden on the callers because many already >> > > > should have backedges marked. What you could instead do is >> > > > add sth like >> > > > >> > > > if (flag_checking) >> > > > { >> > > > auto_edge_flag saved_dfs_back; >> > > > for-each-edge-in-cfg () set saved_dfs_back flag if dfs_back is >> > > > set, clear dfs_back >> > > > mark_dfs_back_edges (); >> > > > for-each-edge-in-cfg () verify the flags are set on the same >> > > > edges and clear saved_dfs_back >> > > > } >> > > > >> > > > to the path_range_query constructor. That way we at least notice when passes >> > > > do _not_ have the backedges marked properly. >> > > >> > > Sounds good. Thanks. >> > > >> > > I've put the verifier by mark_dfs_back_edges(), since it really has >> > > nothing to do with the path solver. Perhaps it's useful for someone >> > > else. >> > > >> > > The patch triggered with the loop-ch use, so I've added a >> > > corresponding mark_dfs_back_edges there. >> > > >> > > OK pending tests? >> > >> > Please rename dfs_back_edges_available_p to sth not suggesting >> > it's a simple predicate check, like maybe verify_marked_backedges. >> > >> > + >> > + gcc_checking_assert (!m_resolve || dfs_back_edges_available_p ()); >> > >> > I'd prefer the following which allows even release checking builds >> > to verify this with -fchecking. >> > >> > if (!m_resolve) >> > if (flag_checking) >> > verify_marked_backedges (); >> > >> > and then ideally verify_marked_backedges () should raise an >> > internal_error () for the edge not marked properly, best by >> > also specifying it. Just like other CFG verifiers do. >> > >> > The loop ch and backwards threader changes are OK. You >> > can post the verification independently again. >> >> How about this? >> >> Tested on x86-64 Linux.