From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by sourceware.org (Postfix) with ESMTPS id 8B39B3854833 for ; Wed, 9 Jun 2021 11:48:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8B39B3854833 Received: by mail-ed1-x52b.google.com with SMTP id s6so28303504edu.10 for ; Wed, 09 Jun 2021 04:48:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8k4+PisKanje7/d63gVvZF6bApAYqf5Gn7ioGgOtKFA=; b=jOkfTHVDRj4zaxp0KqxEya7lnNqkbh6w/ZFhmaMSOK16nydhTQCCPZQsHuUD90ahld r3jJawwk15ohZjTRhkPU/j9cgYUmKGfan+UwwBvFT4NJbzqmpftpmBkkO9otukJrrGpO SqsprXQHLZqq66VzES2j76/V52OSa2roiuFmI910hnVFitNQ6JVgc8ZAwpjD/tkrS0VU Cb7UR0Z3nC+xTilwRbB/AMqCKPwQxj6eDvjeZU2u56GaN3Cc/9h9xBlxBgJGLeRzRLCK B7Zww4irZDvaUSC5/ryG2+1n82oBsMrmNsz5znksIrZngwBZo/ELe/Agu3x5pJA8yNHn 6RGw== X-Gm-Message-State: AOAM532L46uEzHi3wI6FFdJ9bjANGV9Yx3ts6Gg/0uYtcWDSHpKQVF3C XV5z68nEJWYVcllsONJZwNcDw+0G6AA/Z+2dZHdywsFYviW+IQ== X-Google-Smtp-Source: ABdhPJyaCvvpm54VwgXpMmC1fdK0r4zo9jbx5GjefUrS9Eu6UVklKP3kxn+mxJDWug1Sfg233LizZ8M0oJCCbWYAZtI= X-Received: by 2002:a05:6402:885:: with SMTP id e5mr30727030edy.248.1623239315484; Wed, 09 Jun 2021 04:48:35 -0700 (PDT) MIME-Version: 1.0 References: <9250230f-a9ff-c30f-4fad-9f7fdeaf52ec@redhat.com> <041880b5-f816-19eb-3fdc-d7e3d77a2578@redhat.com> In-Reply-To: <041880b5-f816-19eb-3fdc-d7e3d77a2578@redhat.com> From: Richard Biener Date: Wed, 9 Jun 2021 13:48:24 +0200 Message-ID: Subject: Re: [PATCH] PR tree-optimization/100781 - Do not calculate new values when evaluating a debug, statement. To: Andrew MacLeod Cc: gcc-patches , Jakub Jelinek Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 09 Jun 2021 11:48:38 -0000 On Tue, Jun 8, 2021 at 4:48 PM Andrew MacLeod wrote: > > On 6/2/21 3:29 AM, Richard Biener wrote: > > On Tue, Jun 1, 2021 at 4:24 PM Andrew MacLeod wrote: > >> On 6/1/21 3:34 AM, Richard Biener wrote: > >>> On Tue, Jun 1, 2021 at 3:38 AM Andrew MacLeod via Gcc-patches > >>> wrote: > >>>> An ongoing issue is the the order we evaluate things in can affect > >>>> decisions along the way. As ranger isn't a fully iterative pass, we can > >>>> sometimes come up with different results if back edges are processed in > >>>> different orders. > >>>> > >>>> One of the ways this can happen is when the cache is propagating > >>>> on-entry values for an SSA_NAME. It calculates outgoing edge values and > >>>> the gori-compute engine can flag ssa-names that were involved in a range > >>>> calculation that have not yet been initialized. When the propagation > >>>> for the original name is done, it goes back and examines the "poor > >>>> values" and tries to quickly calculate a better range, and if it comes > >>>> up with one, immediately tries to go back and update the location/range > >>>> gori_compute flagged. This produces better ranges earlier. > >>>> > >>>> However, when we do this in different orders, we can get different > >>>> results. We were processing the uses on is_gimple_debug statements just > >>>> like normal uses, and this would sometimes cause a difference in how > >>>> things were resolved. > >>>> > >>>> This patch adds a flag to enable/disable this attempt to look up new > >>>> values, and when range_of_expr is processing the use on a debug > >>>> statement, turns it off for the query. This means the query will never > >>>> cause a new lookup, and this should resolve all the -fcompare-debug issues. > >>>> > >>>> Bootstrapped on x86_64-pc-linux-gnu, with no new regressions. Pushed. > >>> Please check if such fixes also apply to the GCC 11 branch. > >>> > >>> Richard. > >>> > >>> > >> I've checked both testcases against gcc11 release, and neither is an > >> issue there. Much of this was triggered by changes to the export list. > >> That said, is there potential for it to surface? The potential is > >> probably there. We'd have to address it differently tho. For the > >> gcc11 release, since we always run in hybrid mode it doesn't really > >> matter if ranger looks up ranges for debug statements... EVRP will still > >> pick up what we use to get for them. we could simply disable looking > >> for contextual ranges for is_gimple_stmt and simply pick up the best > >> known global/on-entry value available.. I can either provide a patch > >> for that now, or deal with it if we ever get a PR. I'm ok either way. > > I think it would be good to robustify the code even w/o a PR. > > > >> btw, when is the next point release? I added an infrastructure patch to > >> trunk (https://gcc.gnu.org/pipermail/gcc-patches/2021-May/569884.html) > >> to enable replacing the on-entry cache to deal with memory consumption > >> issues like in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100299 . I > >> specifically put it in early before the other changes so that it could > >> be directly applied to gcc11 as well, but I need to follow up with one > >> of the replacements I have queued up to look at if we are interested in > >> fixing this in gcc 11. I'll bump the priority to try to hit the next > >> release if thats the case. > > The first point release is usuall about two month from the initial release > > which means in about a month and a half. It would be nice to fix > > those issues and the earlier in the release series the better. > > > > Richard. > > > >> Andrew > >> > OK, so this would be the simple way I'd tackle this in gcc11. This > should be quite safe. Just treat debug_stmts as if they are not stmts.. > and make a global query. EVRP will still provide a contextual range as > good as it ever did, but it wont trigger ranger lookups on debug uses > any more. > > It bootstraps on x86_64-pc-linux-gnu. Is there a process other than > getting the OK to check this into the gcc 11 branch? Does it go into > releases/gcc-11 ? it would go into releases/gcc-11, yes. Now, diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc index 6158a754dd6..fd7fa5e3dbb 100644 --- a/gcc/gimple-range.cc +++ b/gcc/gimple-range.cc @@ -945,7 +945,7 @@ gimple_ranger::range_of_expr (irange &r, tree expr, gimple *stmt) return get_tree_range (r, expr); // If there is no statement, just get the global value. - if (!stmt) + if (!stmt || is_gimple_debug (stmt)) { unfortunately the function is not documented so I'm just guessing here - why do we end up passing in a debug stmt as 'stmt'? (how should expr and stmt relate?) So isn't it better to do this check before if (!gimple_range_ssa_p (expr)) return get_tree_range (r, expr); or even more better, assert we don't get a debug stmt here and fixup whoever calls range_of_expr to not do that for debug stmts? When I add this assertion not even libgcc can configure... backtraces look like #0 fancy_abort (file=0x2a71420 "../../src/gcc-11-branch/gcc/gimple-range.cc", line=944, function=0x2a71638 "range_of_expr") at ../../src/gcc-11-branch/gcc/diagnostic.c:1884 #1 0x0000000001f28275 in gimple_ranger::range_of_expr (this=0x3274eb0, r=..., expr=, stmt=) at ../../src/gcc-11-branch/gcc/gimple-range.cc:944 #2 0x000000000151ab7c in range_query::value_of_expr (this=0x3274eb0, name=, stmt=) at ../../src/gcc-11-branch/gcc/value-query.cc:86 #3 0x0000000001f36ce3 in hybrid_folder::value_of_expr (this=0x7fffffffd990, op=, stmt=) at ../../src/gcc-11-branch/gcc/gimple-ssa-evrp.c:235 #4 0x0000000001387804 in substitute_and_fold_engine::replace_uses_in ( this=0x7fffffffd990, stmt=) at ../../src/gcc-11-branch/gcc/tree-ssa-propagate.c:871 so after EVRP we substitute and fold - but note we're not expecting to do any more analysis in this phase but simply use the computed lattice, since we don't substitute in unreachable code regions and thus SSA form is temporarily broken that might otherwise cause issues. But yes, substitute and fold does substitute into debug stmts (but we don't analyze debug stmts). So maybe somehow arrange for the substitute_and_fold phase to always only use global ranges? Maybe add the ability to "lock" a ranger instance (disabling any further on-demand processing)? Anyway, inheritance is a bit mazy here, so the patch does look sensible. Richard. > Andrew > > >