On Thu, 2013-07-25 at 15:08 +0200, Martin Jambor wrote: > Hi, > > I don't know why it's me again but again I do have a few comments.
Thanks for looking over the patch. > One global remark first: If we are going to start using the gcc > namespace (I understand it you need for isolation of symbols once you > use gcc as library, right?), I'm wondering whether mixing "using > namespace gcc" and explicit identifier qualifications is a good idea. > We have never had a discussion about namespace conventions but I'd > suggest that you add the using directive at the top of all gcc source > files that need it and never use explicit gcc:: qualification (unless > there is a reason for it, of course, like in symbol definitions). > > But perhaps someone else with more C++ experience disagrees? http://gcc.gnu.org/codingconventions.html#Namespace_Use says: > "Namespaces are encouraged. All separable libraries should have a unique global namespace." [...snip...] > "Header files should have neither using directives nor namespace-scope using declarations." and the rationale doc says: > "Using them within an implementation file can help conciseness." However, there doesn't seem to be a discussion on the merits of the various forms of "using" directives. These aren't the GCC coding conventions, but the Google conventions: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces forbid using directives of the form: using NAMESPACE; but suggest using directives of the form: using NAMESPACE::NAME; to pick out individual names from a namespace, though *not* in global scope in a header (to avoid polluting the global namespace). I like this approach - how about using it for frequently used names in a .c/.cc file, keeping the names alphabetizing by "fully qualified path", so e.g.: #include "foo.h" ... #include "bar.h" using gcc::context; using gcc::pass_manager; ...etc, individually grabbing the names we'll be needing // code follows and thus avoiding grabbing whole namespaces, whilst keeping the code concise. I may want to violate that rule within gtype-desc.c, as per: http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01262.html to simplify handling of GTY and "gcc::" > A few more comments inline: Likewise [...] > > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > > index b82c2e0..dc489fb 100644 > > --- a/gcc/cgraphunit.c > > +++ b/gcc/cgraphunit.c > > @@ -194,6 +194,8 @@ along with GCC; see the file COPYING3. If not see > > #include "except.h" > > #include "cfgloop.h" > > #include "regset.h" /* FIXME: For reg_obstack. */ > > +#include "context.h" > > +#include "pipeline.h" > > > > /* Queue of cgraph nodes scheduled to be added into cgraph. This is a > > secondary queue used during optimization to accommodate passes that > > @@ -478,6 +480,7 @@ cgraph_finalize_function (tree decl, bool nested) > > void > > cgraph_add_new_function (tree fndecl, bool lowered) > > { > > + gcc::pipeline &passes = g->get_passes (); > > struct cgraph_node *node; > > switch (cgraph_state) > > { > > @@ -508,7 +511,7 @@ cgraph_add_new_function (tree fndecl, bool lowered) > > push_cfun (DECL_STRUCT_FUNCTION (fndecl)); > > gimple_register_cfg_hooks (); > > bitmap_obstack_initialize (NULL); > > - execute_pass_list (all_lowering_passes); > > + execute_pass_list (passes.all_lowering_passes); > > execute_pass_list (pass_early_local_passes.pass.sub); > > bitmap_obstack_release (NULL); > > pop_cfun (); > > @@ -640,7 +643,7 @@ analyze_function (struct cgraph_node *node) > > > > gimple_register_cfg_hooks (); > > bitmap_obstack_initialize (NULL); > > - execute_pass_list (all_lowering_passes); > > + execute_pass_list (g->get_passes ().all_lowering_passes); > > free_dominance_info (CDI_POST_DOMINATORS); > > free_dominance_info (CDI_DOMINATORS); > > compact_blocks (); > > @@ -1588,7 +1591,7 @@ expand_function (struct cgraph_node *node) > > /* Signal the start of passes. */ > > invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL); > > > > - execute_pass_list (all_passes); > > + execute_pass_list (g->get_passes ().all_passes); > > > > /* Signal the end of passes. */ > > invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL); > > @@ -1807,6 +1810,8 @@ output_in_order (void) > > static void > > ipa_passes (void) > > { > > + gcc::pipeline &passes = g->get_passes (); > > + > > set_cfun (NULL); > > current_function_decl = NULL; > > gimple_register_cfg_hooks (); > > @@ -1816,7 +1821,7 @@ ipa_passes (void) > > > > if (!in_lto_p) > > { > > - execute_ipa_pass_list (all_small_ipa_passes); > > + execute_ipa_pass_list (passes.all_small_ipa_passes); > > if (seen_error ()) > > return; > > } > > @@ -1843,14 +1848,15 @@ ipa_passes (void) > > cgraph_process_new_functions (); > > > > execute_ipa_summary_passes > > - ((struct ipa_opt_pass_d *) all_regular_ipa_passes); > > + ((struct ipa_opt_pass_d *) passes.all_regular_ipa_passes); > > } > > > > /* Some targets need to handle LTO assembler output specially. */ > > if (flag_generate_lto) > > targetm.asm_out.lto_start (); > > > > - execute_ipa_summary_passes ((struct ipa_opt_pass_d *) > > all_lto_gen_passes); > > + execute_ipa_summary_passes ((struct ipa_opt_pass_d *) > > + passes.all_lto_gen_passes); > > Hehe. I understand you have a lot of work with this, but perhaps you > could also turn struct opt_pass and its "descendants" into a struct > hierarchy too? (I'd really use structs, not classes, and for now put > off any other re-engineering like visibilities and such). And put > them into pipeline.h? But perhaps not, I'll try to remember to do it > later :-) I do this in http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01252.html > > if (!in_lto_p) > > ipa_write_summaries (); > > @@ -1859,7 +1865,7 @@ ipa_passes (void) > > targetm.asm_out.lto_end (); > > > > if (!flag_ltrans && (in_lto_p || !flag_lto || flag_fat_lto_objects)) > > - execute_ipa_pass_list (all_regular_ipa_passes); > > + execute_ipa_pass_list (passes.all_regular_ipa_passes); > > invoke_plugin_callbacks (PLUGIN_ALL_IPA_PASSES_END, NULL); > > > > bitmap_obstack_release (NULL); > > @@ -1985,7 +1991,7 @@ compile (void) > > > > cgraph_materialize_all_clones (); > > bitmap_obstack_initialize (NULL); > > - execute_ipa_pass_list (all_late_ipa_passes); > > + execute_ipa_pass_list (g->get_passes ().all_late_ipa_passes); > > symtab_remove_unreachable_nodes (true, dump_file); > > #ifdef ENABLE_CHECKING > > verify_symtab (); > > diff --git a/gcc/context.c b/gcc/context.c > > index 76e0dde..8ec2e60 100644 > > --- a/gcc/context.c > > +++ b/gcc/context.c > > @@ -22,6 +22,12 @@ along with GCC; see the file COPYING3. If not see > > #include "coretypes.h" > > #include "ggc.h" > > #include "context.h" > > +#include "pipeline.h" > > > > /* The singleton holder of global state: */ > > gcc::context *g; > > + > > +gcc::context::context() > > +{ > > + passes_ = new gcc::pipeline(this); > > +} > > Missing spaces before parentheses (yeah, I dislike them too, but...) I somehow got it into my head that constructors were an exception to this rule, based on looking at the base-class constructor examples here: http://gcc.gnu.org/codingconventions.html#Member_Form which lack a space before parens. But yes, I'll ensure that "new CLASSNAME ()" invocations have the space. > > diff --git a/gcc/context.h b/gcc/context.h > > index 3caf02f..a83f7b2 100644 > > --- a/gcc/context.h > > +++ b/gcc/context.h > > @@ -22,14 +22,23 @@ along with GCC; see the file COPYING3. If not see > > > > namespace gcc { > > > > +class pipeline; > > + > > /* GCC's internal state can be divided into zero or more > > "parallel universe" of state; an instance of this class is one such > > context of state. */ > > class context > > { > > public: > > + context(); > > + > > + /* Pass-management. */ > > + > > + pipeline &get_passes () { gcc_assert (passes_); return *passes_; } > > I don't like that you return a reference instead of a pointer. I > believe that in a project like gcc where we are about to mix C++ code > with large portion of original C stuff, we should always strongly > prefer good old pointers to references to avoid confusion. Especially > in return value types. (Yeah, I know that in some cases there are > substantial reasons to return references but this does not seem to be > one of them.) Note that as per http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01365.html we'll use "pass_manager" rather than "pipeline", so this would look like: pass_manager &get_passes () { gcc_assert (passes_); return *passes_; } We were chatting about C++ references on IRC on Friday, and IIRC there was a strong objection to passing *arguments* that are non-const references, rather than return values - mutation of *arguments* being surprising and a place for bugs to arise (though that may have been Diego who was arguing that, citing http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Arguments ). I prefer to have get_passes return a reference rather a pointer since which thing is being referred to isn't going to change, and is non-NULL. One could write it as a "gcc::pipeline const * passes", but that doesn't capture the non-NULL-ness of it. [within the class data, "passes_" needs to be a *pointer* so that the PCH deserialization can work, in case the object has been relocated]. Having the get_passes method do the assertion of non-NULLness and dereference means that there's a single place where the non-NULLness is asserted. I guess this is a bigger point though: how do GCC maintainers feel about C++ references in general? Looking at the GCC Coding Conventions: http://gcc.gnu.org/codingconventions.html and http://gcc.gnu.org/codingrationale.html I don't see any mention of C++ references. Are C++ references permissible inside GCC's own code (outside of libstdc++, of course) and is the above usage sane? There is another question here, which is how people feel about accessors/getters? I deliberately used one here since at my Cauldron talk someone (Roland?) pointed out that if we want to optimize the single-state build, we can change the insides of this method in one place and e.g. put the pass manager in a fixed location in the bss segment, at which point field accesses are of fixed locations, which is far cleaner that the various macro based approaches I had proposed. (how would this interact with references vs pointers, if at all?) > > - /* Currently empty. */ > > +private: > > + /* Pass-management. */ > > + pipeline *passes_; > > > > }; // class context > > > > diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c > > index 4a287f6..d322d9a 100644 > > --- a/gcc/lto-cgraph.c > > +++ b/gcc/lto-cgraph.c > > @@ -47,6 +47,8 @@ along with GCC; see the file COPYING3. If not see > > #include "gcov-io.h" > > #include "tree-pass.h" > > #include "profile.h" > > +#include "context.h" > > +#include "pipeline.h" > > > > static void output_cgraph_opt_summary (void); > > static void input_cgraph_opt_summary (vec<symtab_node> nodes); > > @@ -936,6 +938,7 @@ input_node (struct lto_file_decl_data *file_data, > > enum LTO_symtab_tags tag, > > vec<symtab_node> nodes) > > { > > + gcc::pipeline &passes = g->get_passes (); > > The same here and at a few other places. It may be just me not being > used to references... nevertheless, if someone really wants to use > them like this, at least make them const and you will save a night of > frantic debugging to someone, probably to yourself. But I strongly > prefer pointers... it's hard to describe how strongly I prefer them. OK. How do others feel? As I said above, I like the above idiom, since it puts the assertion of non-NULLness in a single place. FWIW, I've changed the above to use a "using gcc::pass_manager", so in my working copy it currently reads: pass_manager &passes = g->get_passes (); [...snip...] > > diff --git a/gcc/pipeline.h b/gcc/pipeline.h > > new file mode 100644 > > index 0000000..37c90d7 > > --- /dev/null > > +++ b/gcc/pipeline.h > > @@ -0,0 +1,89 @@ > > +/* pipeline.h - The pipeline of optimization passes > > + Copyright (C) 2013 Free Software Foundation, Inc. > > + > > +This file is part of GCC. > > + > > +GCC is free software; you can redistribute it and/or modify it under > > +the terms of the GNU General Public License as published by the Free > > +Software Foundation; either version 3, or (at your option) any later > > +version. > > + > > +GCC is distributed in the hope that it will be useful, but WITHOUT ANY > > +WARRANTY; without even the implied warranty of MERCHANTABILITY or > > +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > +for more details. > > + > > +You should have received a copy of the GNU General Public License > > +along with GCC; see the file COPYING3. If not see > > +<http://www.gnu.org/licenses/>. */ > > + > > +#ifndef GCC_PIPELINE_H > > +#define GCC_PIPELINE_H > > + > > +class opt_pass; > > +struct register_pass_info; > > + > > +/* Define a list of pass lists so that both passes.c and plugins can easily > > + find all the pass lists. */ > > +#define GCC_PASS_LISTS \ > > + DEF_PASS_LIST (all_lowering_passes) \ > > + DEF_PASS_LIST (all_small_ipa_passes) \ > > + DEF_PASS_LIST (all_regular_ipa_passes) \ > > + DEF_PASS_LIST (all_lto_gen_passes) \ > > + DEF_PASS_LIST (all_passes) > > + > > +#define DEF_PASS_LIST(LIST) PASS_LIST_NO_##LIST, > > +enum pass_list > > An enum is a list? I understand this is nit-picking in moved existing > code but I'd change it to something less misleading anyway. Is it > impossible to keep the union name-less for some reason? Is anyone actually using the pass lists? > > +{ > > + GCC_PASS_LISTS > > + PASS_LIST_NUM > > +}; > > +#undef DEF_PASS_LIST > > + > > +namespace gcc { > > + > > +class context; > > + > > +class pipeline > > +{ > > +public: > > + pipeline(context *ctxt); > > + > > + void register_pass (struct register_pass_info *pass_info); > > + void register_one_dump_file (struct opt_pass *pass); > > + > > + opt_pass *get_pass_for_id (int id) const; > > + > > + void dump_passes () const; > > + > > + void dump_profile_report () const; > > + > > +public: > > Extra public? Or do we want that to divide data members from methods? I added it as a marker to separate methods from data members, but I suspect I'm, ahem, "setting precedent" here. http://gcc.gnu.org/codingconventions.html#Class_Form lists the order in which things should be declared within a class. > Thanks for all the work and sorry for the nagging. However, you might > be setting up quite a few C++ precedents so I thought it would be > better to pay attention :-) Agreed - thanks for looking through the patch. Dave
