From patchwork Fri Nov 13 13:57:46 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 544309 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 9F535141405 for ; Sat, 14 Nov 2015 00:58:42 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=ZB/TDOsZ; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=DsQo+bbjReeddBrgT nEtAA+zuLPEiXQSkRBOGRWPWaEtWgOfjbkn7LxBEv3iYp0HExAWGs7v9KxiMxuM+ Z/xAgRQc+xKIZCgp8mj+0lUEPOQx+1G0BFSy7df2FXY0465kdHi55W4Xo5vQV1An eGeRttVlmm5oV78ClM9OCQj9z4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=hdClJ33HvQnOJDSX/t8KffA TUwU=; b=ZB/TDOsZcRMDVVoLbDFfvgjnmFNCt2F5A/G4NPI16tWcDSpWGViPe7u oYj+PWayCFwICypRuiUTmdH+z1s/wvU14RH0zf9mHEaBmNcjJ+wOaD9v6MwIRH57 8XwBPFkkKcJbaLJViJlBv+NbbyzSZOXs2g/zR3hlYUQldj9+HcEI= Received: (qmail 100328 invoked by alias); 13 Nov 2015 13:58:35 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 100312 invoked by uid 89); 13 Nov 2015 13:58:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: fencepost.gnu.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (208.118.235.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 13 Nov 2015 13:58:30 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55340) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1ZxEs0-0004fq-0l for gcc-patches@gnu.org; Fri, 13 Nov 2015 08:58:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZxErt-0003u8-FG for gcc-patches@gnu.org; Fri, 13 Nov 2015 08:58:27 -0500 Received: from relay1.mentorg.com ([192.94.38.131]:41368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxErt-0003u1-32 for gcc-patches@gnu.org; Fri, 13 Nov 2015 08:58:21 -0500 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ZxErr-0006lM-AM from Tom_deVries@mentor.com ; Fri, 13 Nov 2015 05:58:19 -0800 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Fri, 13 Nov 2015 13:58:17 +0000 Subject: [PATCH] Remove first_pass_instance from pass_vrp To: Richard Biener , David Malcolm References: <56447A09.4070608@mentor.com> <564498CE.5010207@mentor.com> <1447342432.7830.21.camel@surprise> CC: "gcc-patches@gnu.org" From: Tom de Vries Message-ID: <5645EC5A.9060005@mentor.com> Date: Fri, 13 Nov 2015 14:57:46 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: Windows NT kernel [generic] [fuzzy] X-Received-From: 192.94.38.131 On 13/11/15 11:35, Richard Biener wrote: > On Thu, Nov 12, 2015 at 4:33 PM, David Malcolm wrote: >> On Thu, 2015-11-12 at 15:06 +0100, Richard Biener wrote: >>> On Thu, Nov 12, 2015 at 3:04 PM, Richard Biener >>> wrote: >>>> On Thu, Nov 12, 2015 at 2:49 PM, Tom de Vries wrote: >>>>> On 12/11/15 13:26, Richard Biener wrote: >>>>>> >>>>>> On Thu, Nov 12, 2015 at 12:37 PM, Tom de Vries >>>>>> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> [ See also related discussion at >>>>>>> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00452.html ] >>>>>>> >>>>>>> this patch removes the usage of first_pass_instance from pass_vrp. >>>>>>> >>>>>>> the patch: >>>>>>> - limits itself to pass_vrp, but my intention is to remove all >>>>>>> usage of first_pass_instance >>>>>>> - lacks an update to gdbhooks.py >>>>>>> >>>>>>> Modifying the pass behaviour depending on the instance number, as >>>>>>> first_pass_instance does, break compositionality of the pass list. In >>>>>>> other >>>>>>> words, adding a pass instance in a pass list may change the behaviour of >>>>>>> another instance of that pass in the pass list. Which obviously makes it >>>>>>> harder to understand and change the pass list. [ I've filed this issue as >>>>>>> PR68247 - Remove pass_first_instance ] >>>>>>> >>>>>>> The solution is to make the difference in behaviour explicit in the pass >>>>>>> list, and no longer change behaviour depending on instance number. >>>>>>> >>>>>>> One obvious possible fix is to create a duplicate pass with a different >>>>>>> name, say 'pass_vrp_warn_array_bounds': >>>>>>> ... >>>>>>> NEXT_PASS (pass_vrp_warn_array_bounds); >>>>>>> ... >>>>>>> NEXT_PASS (pass_vrp); >>>>>>> ... >>>>>>> >>>>>>> But, AFAIU that requires us to choose a different dump-file name for each >>>>>>> pass. And choosing vrp1 and vrp2 as new dump-file names still means that >>>>>>> -fdump-tree-vrp no longer works (which was mentioned as drawback here: >>>>>>> https://gcc.gnu.org/ml/gcc-patches/2012-07/msg00453.html ). >>>>>>> >>>>>>> This patch instead makes pass creation parameterizable. So in the pass >>>>>>> list, >>>>>>> we use: >>>>>>> ... >>>>>>> NEXT_PASS_WITH_ARG (pass_vrp, true /* warn_array_bounds_p */); >>>>>>> ... >>>>>>> NEXT_PASS_WITH_ARG (pass_vrp, false /* warn_array_bounds_p */); >>>>>>> ... >>>>>>> >>>>>>> This approach gives us clarity in the pass list, similar to using a >>>>>>> duplicate pass 'pass_vrp_warn_array_bounds'. >>>>>>> >>>>>>> But it also means -fdump-tree-vrp still works as before. >>>>>>> >>>>>>> Good idea? Other comments? >>>>>> >>>>>> >>>>>> It's good to get rid of the first_pass_instance hack. >>>>>> >>>>>> I can't comment on the AWK, leaving that to others. Syntax-wise I'd hoped >>>>>> we can just use NEXT_PASS with the extra argument being optional... >>>>> >>>>> >>>>> I suppose I could use NEXT_PASS in the pass list, and expand into >>>>> NEXT_PASS_WITH_ARG in pass-instances.def. >>>>> >>>>> An alternative would be to change the NEXT_PASS macro definitions into >>>>> vararg variants. But the last time I submitted something with a vararg macro >>>>> ( https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00794.html ), I got a >>>>> question about it ( https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00912.html >>>>> ), so I tend to avoid using vararg macros. >>>>> >>>>>> I don't see the need for giving clone_with_args a new name, just use an >>>>>> overload >>>>>> of clone ()? >>>>> >>>>> >>>>> That's what I tried initially, but I ran into: >>>>> ... >>>>> src/gcc/tree-pass.h:85:21: warning: ‘virtual opt_pass* opt_pass::clone()’ >>>>> was hidden [-Woverloaded-virtual] >>>>> virtual opt_pass *clone (); >>>>> ^ >>>>> src/gcc/tree-vrp.c:10393:14: warning: by ‘virtual opt_pass* >>>>> {anonymous}::pass_vrp::clone(bool)’ [-Woverloaded-virtual] >>>>> opt_pass * clone (bool warn_array_bounds_p) { return new pass_vrp >>>>> (m_ctxt, warn_array_bounds_p); } >>>>> ... >>>>> >>>>> Googling the error message gives this discussion: ( >>>>> http://stackoverflow.com/questions/16505092/confused-about-virtual-overloaded-functions >>>>> ), and indeed adding >>>>> "using gimple_opt_pass::clone;" >>>>> in class pass_vrp makes the warning disappear. >>>>> >>>>> I'll submit an updated version. >>>> >>>> Hmm, but actually the above means the pass does not expose the >>>> non-argument clone >>>> which is good! >>>> >>>> Or did you forget to add the virtual-with-arg variant to opt_pass? >>>> That is, why's it >>>> a virtual function in the first place? (clone_with_arg) >>> >>> That said, >>> >>> --- a/gcc/tree-pass.h >>> +++ b/gcc/tree-pass.h >>> @@ -83,6 +83,7 @@ public: >>> >>> The default implementation prints an error message and aborts. */ >>> virtual opt_pass *clone (); >>> + virtual opt_pass *clone_with_arg (bool); >>> >>> >>> means the arg type is fixed at 'bool' (yeah, mimicing >>> first_pass_instance). That >>> looks a bit limiting to me, but anyway. >>> >>> Richard. >>> >>>>> Thanks, >>>>> - Tom >>>>> >>>>> >>>>>> [ideally C++ would allow us to say that only one overload may be >>>>>> implemented] >> >> IIRC, the idea of the clone vfunc was to support state management of >> passes: to allow all the of the sibling passes within a pass manager to >> be able to locate each other, so they can share state if desired, >> without sharing state with "cousin" passes in another pass manager (for >> a halcyon future in which multiple instances of gcc could be running in >> one process in different threads). >> >> I've changed my mind on the merits of this: I think state should be >> stored in the IR itself, not in the passes themselves. >> >> I don't think we have any implementations of "clone" that don't simply >> call "return new pass_foo ()". >> >> So maybe it makes sense to eliminate clone in favor of being able to >> pass arguments to the factory function (and thence to the ctor); >> something like: >> >> gimple_opt_pass * >> make_pass_vrp (gcc::context *ctxt, bool warn_array_bounds_p) >> { >> return new pass_vrp (ctxt, warn_array_bounds_p); >> } >> >> and then to rewrite passes.c's: >> >> #define NEXT_PASS(PASS, NUM) \ >> do { \ >> gcc_assert (NULL == PASS ## _ ## NUM); \ >> if ((NUM) == 1) \ >> PASS ## _1 = make_##PASS (m_ctxt); \ >> else \ >> { \ >> gcc_assert (PASS ## _1); \ >> PASS ## _ ## NUM = PASS ## _1->clone (); \ >> } \ >> p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ >> } while (0) >> >> to something like: >> >> #define NEXT_PASS(PASS, NUM) \ >> do { \ >> gcc_assert (NULL == PASS ## _ ## NUM); \ >> PASS ## _ ## NUM = make_##PASS (m_ctxt); >> p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ >> } while (0) >> >> or somesuch, and: >> >> #define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \ >> do { \ >> gcc_assert (NULL == PASS ## _ ## NUM); \ >> PASS ## _ ## NUM = make_##PASS (m_ctxt, (ARG)); >> p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ >> } while (0) >> >> Alternatively, if we do want to retain clone, perhaps we could have a >> opt_pass::set_arg vfunc? >> >> virtual void set_arg (bool ) { gcc_unreachable (); } /* provide dummy >> base class impl, but if you're going to use NEXT_PASS_WITH_ARG, you >> really should provide an impl */ >> >> with the subclass implementing it like this, to capture it within a >> field of the >> >> void pass_vrp::set_arg (bool warn_array_bounds_p) >> { >> m_warn_array_bounds_p = warn_array_bounds_p; >> } >> >> and something like this: >> >> #define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \ >> do { \ >> NEXT_PASS (PASS, NUM); \ >> PASS ## _ ## NUM->set_arg (ARG); \ >> } while (0) >> >> or somesuch? >> >> Hope this is constructive > > Yes, and agreed. I've implemented the set_arg scenario, though I've renamed it to set_pass_param. I've also added a parameter number argument to set_pass_param. Furthermore, I've included the gdbhooks.py update. OK for trunk if bootstrap and reg-test passes? Btw, I think NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */); is now equivalent to NEXT_PASS (pass_vrp); I'm not sure which one I prefer in passes.def. Thanks, - Tom Remove first_pass_instance from pass_vrp 2015-11-13 Tom de Vries * gdbhooks.py (class PassNames): Handle extra arg NEXT_PASS argument. * gen-pass-instances.awk (handle_line): Same. * pass_manager.h (class pass_manager): Define and undefine NEXT_PASS_WITH_ARG. * passes.c (opt_pass::set_pass_param): New function. (pass_manager::pass_manager): Define and undefine NEXT_PASS_WITH_ARG. * passes.def: Add extra arg to NEXT_PASS (pass_vrp). * tree-pass.h (gimple_opt::set_pass_param): Declare. * tree-vrp.c (vrp_finalize, execute_vrp): Add and handle warn_array_bounds_p parameter. (pass_vrp::pass_vrp): Initialize warn_array_bounds_p. (pass_vrp::set_pass_param): New function. (pass_vrp::execute): Add warn_array_bounds_p arg to execute_vrp call. (pass_vrp::warn_array_bounds_p): New private member. --- gcc/gdbhooks.py | 2 +- gcc/gen-pass-instances.awk | 28 +++++++++++++++++++++++----- gcc/pass_manager.h | 2 ++ gcc/passes.c | 14 ++++++++++++++ gcc/passes.def | 4 ++-- gcc/tree-pass.h | 1 + gcc/tree-vrp.c | 20 ++++++++++++++------ 7 files changed, 57 insertions(+), 14 deletions(-) diff --git a/gcc/gdbhooks.py b/gcc/gdbhooks.py index 2b9a94c..f920392 100644 --- a/gcc/gdbhooks.py +++ b/gcc/gdbhooks.py @@ -537,7 +537,7 @@ class PassNames: self.names = [] with open(os.path.join(srcdir, 'passes.def')) as f: for line in f: - m = re.match('\s*NEXT_PASS \((.+)\);', line) + m = re.match('\s*NEXT_PASS \(([^,]+).*\);', line) if m: self.names.append(m.group(1)) diff --git a/gcc/gen-pass-instances.awk b/gcc/gen-pass-instances.awk index 9cff429..106a2f6 100644 --- a/gcc/gen-pass-instances.awk +++ b/gcc/gen-pass-instances.awk @@ -61,12 +61,14 @@ function handle_line() len_of_args = len_of_call - (len_of_start + len_of_close); args_start_at = call_starts_at + len_of_start; args_str = substr(line, args_start_at, len_of_args); + split(args_str, args, ","); - # Set pass_name argument - pass_name = args_str; + # Set pass_name argument, an optional with_arg argument + pass_name = args[1]; + with_arg = args[2]; - # Find call expression prefix (until and including called function) - len_of_prefix = args_start_at - 1 - len_of_open; + # Find call expression prefix + len_of_prefix = call_starts_at - 1; prefix = substr(line, 1, len_of_prefix); # Find call expression postfix @@ -82,7 +84,23 @@ function handle_line() pass_num = pass_counts[pass_name]; # Print call expression with extra pass_num argument - printf "%s(%s, %s)%s\n", prefix, pass_name, pass_num, postfix; + printf "%s", prefix; + if (with_arg) + { + printf "NEXT_PASS_WITH_ARG"; + } + else + { + printf "NEXT_PASS"; + } + printf " ("; + printf "%s", pass_name; + printf ", %s", pass_num; + if (with_arg) + { + printf ", %s", with_arg; + } + printf ")%s\n", postfix; } { handle_line() } diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h index 7d539e4..a8199e2 100644 --- a/gcc/pass_manager.h +++ b/gcc/pass_manager.h @@ -120,6 +120,7 @@ private: #define PUSH_INSERT_PASSES_WITHIN(PASS) #define POP_INSERT_PASSES() #define NEXT_PASS(PASS, NUM) opt_pass *PASS ## _ ## NUM +#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) NEXT_PASS (PASS, NUM) #define TERMINATE_PASS_LIST() #include "pass-instances.def" @@ -128,6 +129,7 @@ private: #undef PUSH_INSERT_PASSES_WITHIN #undef POP_INSERT_PASSES #undef NEXT_PASS +#undef NEXT_PASS_WITH_ARG #undef TERMINATE_PASS_LIST }; // class pass_manager diff --git a/gcc/passes.c b/gcc/passes.c index dd8d00a..e634c5c 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -81,6 +81,13 @@ opt_pass::clone () internal_error ("pass %s does not support cloning", name); } +void +opt_pass::set_pass_param (unsigned int, bool) +{ + internal_error ("pass %s needs a set_pass_param implementation to handle the" + " extra argument in NEXT_PASS", name); +} + bool opt_pass::gate (function *) { @@ -1572,6 +1579,12 @@ pass_manager::pass_manager (context *ctxt) p = next_pass_1 (p, PASS ## _ ## NUM, PASS ## _1); \ } while (0) +#define NEXT_PASS_WITH_ARG(PASS, NUM, ARG) \ + do { \ + NEXT_PASS (PASS, NUM); \ + PASS ## _ ## NUM->set_pass_param (0, ARG); \ + } while (0) + #define TERMINATE_PASS_LIST() \ *p = NULL; @@ -1581,6 +1594,7 @@ pass_manager::pass_manager (context *ctxt) #undef PUSH_INSERT_PASSES_WITHIN #undef POP_INSERT_PASSES #undef NEXT_PASS +#undef NEXT_PASS_WITH_ARG #undef TERMINATE_PASS_LIST /* Register the passes with the tree dump code. */ diff --git a/gcc/passes.def b/gcc/passes.def index c0ab6b9..3e23edc 100644 --- a/gcc/passes.def +++ b/gcc/passes.def @@ -171,7 +171,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_return_slot); NEXT_PASS (pass_fre); NEXT_PASS (pass_merge_phi); - NEXT_PASS (pass_vrp); + NEXT_PASS (pass_vrp, true /* warn_array_bounds_p */); NEXT_PASS (pass_chkp_opt); NEXT_PASS (pass_dce); NEXT_PASS (pass_stdarg); @@ -280,7 +280,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_tracer); NEXT_PASS (pass_dominator); NEXT_PASS (pass_strlen); - NEXT_PASS (pass_vrp); + NEXT_PASS (pass_vrp, false /* warn_array_bounds_p */); /* The only const/copy propagation opportunities left after DOM and VRP should be due to degenerate PHI nodes. So rather than run the full propagators, run a specialized pass which diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index 49e22a9..7b2571f 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -83,6 +83,7 @@ public: The default implementation prints an error message and aborts. */ virtual opt_pass *clone (); + virtual void set_pass_param (unsigned int, bool); /* This pass and all sub-passes are executed only if the function returns true. The default implementation returns true. */ diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index e2393e4..5d085b4 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -10183,7 +10183,7 @@ finalize_jump_threads (void) /* Traverse all the blocks folding conditionals with known ranges. */ static void -vrp_finalize (void) +vrp_finalize (bool warn_array_bounds_p) { size_t i; @@ -10199,7 +10199,7 @@ vrp_finalize (void) substitute_and_fold (op_with_constant_singleton_value_range, vrp_fold_stmt, false); - if (warn_array_bounds && first_pass_instance) + if (warn_array_bounds && warn_array_bounds_p) check_all_array_refs (); /* We must identify jump threading opportunities before we release @@ -10289,7 +10289,7 @@ vrp_finalize (void) probabilities to aid branch prediction. */ static unsigned int -execute_vrp (void) +execute_vrp (bool warn_array_bounds_p) { int i; edge e; @@ -10313,7 +10313,7 @@ execute_vrp (void) vrp_initialize (); ssa_propagate (vrp_visit_stmt, vrp_visit_phi_node); - vrp_finalize (); + vrp_finalize (warn_array_bounds_p); free_numbers_of_iterations_estimates (cfun); @@ -10386,14 +10386,22 @@ class pass_vrp : public gimple_opt_pass { public: pass_vrp (gcc::context *ctxt) - : gimple_opt_pass (pass_data_vrp, ctxt) + : gimple_opt_pass (pass_data_vrp, ctxt), warn_array_bounds_p (false) {} /* opt_pass methods: */ opt_pass * clone () { return new pass_vrp (m_ctxt); } + void set_pass_param (unsigned int n, bool param) + { + gcc_assert (n == 0); + warn_array_bounds_p = param; + } virtual bool gate (function *) { return flag_tree_vrp != 0; } - virtual unsigned int execute (function *) { return execute_vrp (); } + virtual unsigned int execute (function *) + { return execute_vrp (warn_array_bounds_p); } + private: + bool warn_array_bounds_p; }; // class pass_vrp } // anon namespace