From patchwork Wed Apr 3 16:13:05 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 233553 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 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "localhost", Issuer "www.qmailtoaster.com" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 16E6E2C00EF for ; Thu, 4 Apr 2013 03:13:02 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:to:date:content-type:mime-version; q= dns; s=default; b=Ee+U7nz8jGVN+Mnv9LmHmu/EIjzesxWJ5Z2v4p9M2Sx9Ya P0bEdEwCIAnTFhAdiiiVkLG7veDwCFLLPP6y2dWG5XtwAkSzc7zEfyii/P2RABsk pR5FS9Sacdx/ByBkGGNVZG9vxEIlJr19VvdR77Z58RgkiKprJR4LvAIB4ptxg= 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 :message-id:subject:from:to:date:content-type:mime-version; s= default; bh=VxaL7Sqems3i8JxNp9Xx/ert9BM=; b=Od79gBtNy2Wts41cVjQx HyjZMrnhj8byo9/PQp6k8HAqXjA1QcjU/WFmeQJEwo0KssSgBmdaX9c8v00KLhTF mFrWBXqgX8YjcYFFrtY1juiWbGke3c3IGj+uizLRIgkovIIIFaind1ntriCfOGEW B4viQd70h4Y/BFk3x+qz1+g= Received: (qmail 22510 invoked by alias); 3 Apr 2013 16:12:48 -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 22463 invoked by uid 89); 3 Apr 2013 16:12:41 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 03 Apr 2013 16:12:41 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r33GCe9d021878 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 3 Apr 2013 12:12:40 -0400 Received: from [10.16.189.30] (dhcp-189-30.bos.redhat.com [10.16.189.30]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r33GCdUq019539 for ; Wed, 3 Apr 2013 12:12:40 -0400 Message-ID: <1365005585.24820.35.camel@surprise> Subject: RFC: elimination of global state relating to passes From: David Malcolm To: gcc-patches@gcc.gnu.org Date: Wed, 03 Apr 2013 12:13:05 -0400 Mime-Version: 1.0 X-Virus-Found: No I'm working on my first gcc contribution, but it's a large patch, and I wanted to sound things out on this list. I want to eliminate/minimize global state within gcc, since I think doing so is a key part of making gcc more modular. Currently there's a lot of global state associated with passes: * the pass tree itself: a single global tree of passes, with callbacks ("gate" and "execute") * globals within individual pass .c files My plan is to instead have the passes be dynamically created instances of pass subclasses, where each pass has a custom subclass of the appropriate pass class: So, for example, mudflap would become something like: class pass_mudflap_2 : public gimple_opt_pass { public: bool gate() { return gate_mudflap(); } unsigned int execute() { return execute_mudflap_function_ops(); } }; where these subclasses are hidden inside the .c files (in this case inside gcc/tree-mudflap.c). All that's exposed to the headers would then be a factory function: gimple_opt_pass * make_pass_mudflap_2 (context &ctxt) { return new pass_mudflap_2 (ctxt); } Globals within a .c file that are specific to a particular pass may then be movable to instance data of the pass subclass (on a case-by-case basis). Each pass also has a reference back to a new "context" class, shared by all the passes: this is currently empty, but I see it being useful as a place to add any global state that isn't part of the pass itself: given that this patch is touching hundreds of files I'd rather add it now, rather than having to go back and add it later. I started doing this by hand, however, there are hundreds of passes, so to avoid lots of error-prone typing I've written a refactoring script. You can see the test suite for the refactoring script here: https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_refactor.py which should give a better idea of the before/after when applying the refactoring (and the use of a test suite hopefully increases the credibility that the resulting patch doesn't change the effective behavior of the code). When run, the refactoring script generates this 107 files changed, 5008 insertions(+), 4214 deletions(-) There's also a hand-written part of the patch, which I'm attaching the most pertinent parts of i.e. gcc/tree-pass.h It doesn't quite compile yet, gcc/passes.c needs work - in particular the code to construct and manage the tree of passes (I think I can do this, but it may need moving the bulk of init_optimization_passes() into a new passes.def file so that I can include it again elsewhere with a different defintion of NEXT_PASS). There's also a new "pipeline" class representing the pass hierarchy, which stores the pointers to the pass instances, so that it's easy to access them from gdb when debugging. One wart in my plan is that a lot of the existing callbacks are checked against NULL and if non-NULL, then extra things happen before/after the call. I don't know of a portable way to do this for a C++ virtual function, so each callback becomes *two* vtable entries: bool has_FOO() // equivalent to (pass->FOO != NULL) in old code and impl_FOO() // equivalent to (pass->FOO ()) in old code I'm currently working off of this git repo: git://gcc.gnu.org/git/gcc.git in the hope of getting this into 4.9 during stage 1. Hope this is constructive Dave diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index 0942ad7..d45b6a5 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -25,6 +25,17 @@ along with GCC; see the file COPYING3. If not see #include "timevar.h" #include "dumpfile.h" +class context; +class pipeline; +class opt_pass; + +/* Holder for state */ +class context +{ +public: + pipeline *passes; +}; + /* Optimization pass type. */ enum opt_pass_type { @@ -34,10 +45,80 @@ enum opt_pass_type IPA_PASS }; +/* Sets of properties input and output from this pass. */ +struct pass_properties +{ +public: + pass_properties(unsigned int required, + unsigned int provided, + unsigned int destroyed) + : required(required), + provided(provided), + destroyed(destroyed) + {} + + unsigned int required; + unsigned int provided; + unsigned int destroyed; +}; + +/* Flags indicating common sets things to do before and after a pass. */ +struct pass_todo_flags +{ +public: + pass_todo_flags(unsigned int start, + unsigned int finish) + : start(start), + finish(finish) + {} + + unsigned int start; + unsigned int finish; +}; + /* Describe one pass; this is the common part shared across different pass types. */ -struct opt_pass +class opt_pass { +public: + virtual ~opt_pass () { } + + /* Public Methods */ + + /* This pass and all sub-passes are executed only if + the function returns true. */ + virtual bool has_gate() { return false; } + virtual bool gate() { return true; } + + /* This is the code to run. The return value contains + TODOs to execute in addition to those in TODO_flags_finish. */ + virtual bool has_execute() = 0; + virtual unsigned int impl_execute() = 0; + +protected: + opt_pass(context &ctxt, + enum opt_pass_type type, + const char *name, + unsigned int optinfo_flags, + timevar_id_t tv_id, + const pass_properties &props, + const pass_todo_flags &todo_flags) + : ctxt(ctxt), + type(type), + name(name), + optinfo_flags(optinfo_flags), + sub(NULL), + next(NULL), + static_pass_number(0), + tv_id(tv_id), + props(props), + todo_flags(todo_flags) + {} + +/* We should eventually make these fields private: */ +public: + context &ctxt; + /* Optimization pass type. */ enum opt_pass_type type; @@ -48,15 +129,6 @@ struct opt_pass /* The -fopt-info optimization group flags as defined in dumpfile.h. */ unsigned int optinfo_flags; - /* If non-null, this pass and all sub-passes are executed only if - the function returns true. */ - bool (*gate) (void); - - /* This is the code to run. If null, then there should be sub-passes - otherwise this pass does nothing. The return value contains - TODOs to execute in addition to those in TODO_flags_finish. */ - unsigned int (*execute) (void); - /* A list of sub-passes to run, dependent on gate predicate. */ struct opt_pass *sub; @@ -70,26 +142,48 @@ struct opt_pass /* ??? Ideally would be dynamically assigned. */ timevar_id_t tv_id; - /* Sets of properties input and output from this pass. */ - unsigned int properties_required; - unsigned int properties_provided; - unsigned int properties_destroyed; - - /* Flags indicating common sets things to do before and after. */ - unsigned int todo_flags_start; - unsigned int todo_flags_finish; + pass_properties props; + pass_todo_flags todo_flags; }; /* Description of GIMPLE pass. */ -struct gimple_opt_pass +class gimple_opt_pass : public opt_pass { - struct opt_pass pass; +public: + gimple_opt_pass(context &ctxt, + const char *name, + unsigned int optinfo_flags, + timevar_id_t tv_id, + const pass_properties &props, + const pass_todo_flags &todo_flags) + : opt_pass(ctxt, + GIMPLE_PASS, + name, + optinfo_flags, + tv_id, + props, + todo_flags) + {} }; /* Description of RTL pass. */ -struct rtl_opt_pass +class rtl_opt_pass : public opt_pass { - struct opt_pass pass; +public: + rtl_opt_pass(context &ctxt, + const char *name, + unsigned int optinfo_flags, + timevar_id_t tv_id, + const pass_properties &props, + const pass_todo_flags &todo_flags) + : opt_pass(ctxt, + RTL_PASS, + name, + optinfo_flags, + tv_id, + props, + todo_flags) + {} }; struct varpool_node; @@ -98,42 +192,81 @@ struct lto_symtab_encoder_d; /* Description of IPA pass with generate summary, write, execute, read and transform stages. */ -struct ipa_opt_pass_d +class ipa_opt_pass_d : public opt_pass { - struct opt_pass pass; +public: + ipa_opt_pass_d(context &ctxt, + const char *name, + unsigned int optinfo_flags, + timevar_id_t tv_id, + const pass_properties &props, + const pass_todo_flags &todo_flags, + unsigned int function_transform_todo_flags_start) + : opt_pass(ctxt, + IPA_PASS, + name, + optinfo_flags, + tv_id, + props, + todo_flags), + function_transform_todo_flags_start(function_transform_todo_flags_start) + {} /* IPA passes can analyze function body and variable initializers using this hook and produce summary. */ - void (*generate_summary) (void); + virtual bool has_generate_summary() = 0; + virtual void impl_generate_summary() = 0; /* This hook is used to serialize IPA summaries on disk. */ - void (*write_summary) (void); + virtual bool has_write_summary() = 0; + virtual void impl_write_summary() = 0; /* This hook is used to deserialize IPA summaries from disk. */ - void (*read_summary) (void); + virtual bool has_read_summary() = 0; + virtual void impl_read_summary() = 0; /* This hook is used to serialize IPA optimization summaries on disk. */ - void (*write_optimization_summary) (void); + virtual bool has_write_optimization_summary() = 0; + virtual void impl_write_optimization_summary() = 0; /* This hook is used to deserialize IPA summaries from disk. */ - void (*read_optimization_summary) (void); + virtual bool has_read_optimization_summary() = 0; + virtual void impl_read_optimization_summary() = 0; /* Hook to convert gimple stmt uids into true gimple statements. The second parameter is an array of statements indexed by their uid. */ - void (*stmt_fixup) (struct cgraph_node *, gimple *); + virtual bool has_stmt_fixup() = 0; + virtual void impl_stmt_fixup(struct cgraph_node *, gimple *) = 0; + + virtual bool has_function_transform() = 0; + virtual unsigned int impl_function_transform(struct cgraph_node *) = 0; + + virtual void variable_transform(struct varpool_node *) = 0; /* Results of interprocedural propagation of an IPA pass is applied to function body via this hook. */ unsigned int function_transform_todo_flags_start; - unsigned int (*function_transform) (struct cgraph_node *); - void (*variable_transform) (struct varpool_node *); }; /* Description of simple IPA pass. Simple IPA passes have just one execute hook. */ -struct simple_ipa_opt_pass +class simple_ipa_opt_pass : public opt_pass { - struct opt_pass pass; +public: + simple_ipa_opt_pass(context &ctxt, + const char *name, + unsigned int optinfo_flags, + timevar_id_t tv_id, + const pass_properties &props, + const pass_todo_flags &todo_flags) + : opt_pass(ctxt, + SIMPLE_IPA_PASS, + name, + optinfo_flags, + tv_id, + props, + todo_flags) + {} }; /* Pass properties. */