From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54486 invoked by alias); 17 Jan 2017 09:28:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 54463 invoked by uid 89); 17 Jan 2017 09:28:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=H*i:sk:ecda659, H*f:sk:ecda659 X-HELO: mail-ot0-f193.google.com Received: from mail-ot0-f193.google.com (HELO mail-ot0-f193.google.com) (74.125.82.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Jan 2017 09:28:42 +0000 Received: by mail-ot0-f193.google.com with SMTP id 73so8671088otj.1 for ; Tue, 17 Jan 2017 01:28:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fJUBhYEidNv9jSK8NeUqrSxRyIWs39QsYKQGhkTGJhg=; b=FEqrpV+Hjpx49d4T6GJGDOUYCLn3qrfTqy8rJo0nootQCW0YzocfB4vNw/9O3Ftg0Y 6lBcqrHcF4Eku2gLsFYe6TOJNd043fb3LW/WSwQJJ8eR67pW7fswLFNsMZ9dhvg92GlS 9rV4VPKFornpuZu2hDrwoRX8K7voCYfLkOy8fjdo59XS2nUOolH18sNDm5Fh+kqC7yNj GWpPiL2mSLu6PLHW6TpFcQe5ZcVq1wo1O0pV0PAbtZG5OFjKuEGjuUwHW3/b9lpoBHE8 rQgpG1XzuDBDE5xs/TUx43GOQSL0CXLibQxG6NZx8KtMD4mo2xW22jbShxUwIKzYiZbM lC8Q== X-Gm-Message-State: AIkVDXKECMOol8s8S4bLcYbKWGFitUWbSJx5qTRp+YCnM4DfOJKYXAMtJjq9fpMFnvDvs8wrQ9ond3vFyKpAgw== X-Received: by 10.157.58.101 with SMTP id j92mr10894615otc.100.1484645320978; Tue, 17 Jan 2017 01:28:40 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.42.12 with HTTP; Tue, 17 Jan 2017 01:28:40 -0800 (PST) In-Reply-To: References: <1484015904-32671-1-git-send-email-dmalcolm@redhat.com> <1484015904-32671-6-git-send-email-dmalcolm@redhat.com> From: Richard Biener Date: Tue, 17 Jan 2017 09:28:00 -0000 Message-ID: Subject: Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions To: Jeff Law Cc: David Malcolm , GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg01170.txt.bz2 On Mon, Jan 16, 2017 at 10:42 PM, Jeff Law wrote: > On 01/09/2017 07:38 PM, David Malcolm wrote: >> >> gcc/ChangeLog: >> * passes.c: Include "insn-addr.h". >> (should_skip_pass_p): Add logging. Update logic for running >> "expand" to be compatible with both __GIMPLE and __RTL. Guard >> property-provider override so it is only done for gimple passes. >> Don't skip dfinit. >> (skip_pass): New function. >> (execute_one_pass): Call skip_pass when skipping passes. >> --- >> gcc/passes.c | 65 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 58 insertions(+), 7 deletions(-) >> >> diff --git a/gcc/passes.c b/gcc/passes.c >> index 31262ed..6954d1e 100644 >> --- a/gcc/passes.c >> +++ b/gcc/passes.c >> @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see >> #include "cfgrtl.h" >> #include "tree-ssa-live.h" /* For remove_unused_locals. */ >> #include "tree-cfgcleanup.h" >> +#include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC. */ > > insn-addr? Yuk. > > >> >> using namespace gcc; >> >> @@ -2315,26 +2316,73 @@ should_skip_pass_p (opt_pass *pass) >> if (!cfun->pass_startwith) >> return false; >> >> - /* We can't skip the lowering phase yet -- ideally we'd >> - drive that phase fully via properties. */ >> - if (!(cfun->curr_properties & PROP_ssa)) >> - return false; >> + /* For __GIMPLE functions, we have to at least start when we leave >> + SSA. */ >> + if (pass->properties_destroyed & PROP_ssa) >> + { >> + if (!quiet_flag) >> + fprintf (stderr, "starting anyway when leaving SSA: %s\n", >> pass->name); >> + cfun->pass_startwith = NULL; >> + return false; >> + } > > This seems to need a comment -- it's not obvious how destroying the SSA > property maps to a pass that can not be skipped. >> >> >> >> - /* And also run any property provider. */ >> - if (pass->properties_provided != 0) >> + /* Run any property provider. */ >> + if (pass->type == GIMPLE_PASS >> + && pass->properties_provided != 0) >> return false; > > So comment needed here too. I read this as "if a gimple pass provides a > property then it should not be skipped. Which means that an RTL pass that > provides a property can? > > >> >> + /* Don't skip df init; later RTL passes need it. */ >> + if (strstr (pass->name, "dfinit") != NULL) >> + return false; > > Which seems like a failing in RTL passes saying they need DF init. > > > >> +/* Skip the given pass, for handling passes before "startwith" >> + in __GIMPLE and__RTL-marked functions. >> + In theory, this ought to be a no-op, but some of the RTL passes >> + need additional processing here. */ >> + >> +static void >> +skip_pass (opt_pass *pass) > > ... > This all feels like a failing in how we handle state in the RTL world. And I > suspect it's prone to error. Imagine if I'm hacking on something in the RTL > world and my code depends on something else being set up. I really ought > to have a way within my pass to indicate what I depend on. Having it hidden > away in passes.c makes it easy to miss/forget. > > >> +{ >> + /* Pass "reload" sets the global "reload_completed", and many >> + things depend on this (e.g. instructions in .md files). */ >> + if (strcmp (pass->name, "reload") == 0) >> + reload_completed = 1; > > Seems like this ought to be a property provided by LRA/reload. > > >> + >> + /* The INSN_ADDRESSES vec is normally set up by >> + shorten_branches; set it up for the benefit of passes that >> + run after this. */ >> + if (strcmp (pass->name, "shorten") == 0) >> + INSN_ADDRESSES_ALLOC (get_max_uid ()); > > Similarly ought to be provided by shorten-branches > >> + >> + /* Update the cfg hooks as appropriate. */ >> + if (strcmp (pass->name, "into_cfglayout") == 0) >> + { >> + cfg_layout_rtl_register_cfg_hooks (); >> + cfun->curr_properties |= PROP_cfglayout; >> + } >> + if (strcmp (pass->name, "outof_cfglayout") == 0) >> + { >> + rtl_register_cfg_hooks (); >> + cfun->curr_properties &= ~PROP_cfglayout; >> + } >> +} > > This feels somewhat different, but still a hack. > > I don't have strong suggestions on how to approach this, but what we've got > here feels like a hack and one prone to bitrot. All the above needs a bit of cleanup in the way we use (or not use) PROP_xxx. For example right now you can't startwith a __GIMPLE with a pass inside the loop pipeline because those passes expect loops to be initialized and be in loop-closed SSA. And with the hack above for the property providers you'll always run pass_crited (that's a bad user of a PROP_). Ideally we'd figure out required properties from the startwith pass (but there's not an easy way to compute it w/o actually "executing" the passes) and then enable enough passes on the way to it providing those properties. Or finally restructure things in a way that the pass manager automatically runs property provider passes before passes requiring properties that are not yet available... Instead of those pass->name comparisions we could invent a new flag in the pass structure whether a pass should always be run for __GIMPLE or ___RTL but that's a bit noisy right now. So I'm fine with the (localized) "hacks" for the moment. Richard. > jeff