Hi, thanks for the email I was supposed to write.
On Mon, Jul 29, 2013 at 02:20:02PM -0400, David Malcolm wrote: > 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 don't know. Before your patches there were no namespaces and the one flat global namespace is cluttered, all symbols from included header files are there. I thought you just wanted to move (some part of) this to the gcc namespace so that users of gcc-as-library will not clash with it. So I guess it depends on what you want to move to the gcc namespace. This enumeration approach would be viable if there were only a few things in it, we can't expect people to put tens or hundreds of such using directives to their sources. In any event, I do not consider this to be really important, I was just curious. One day there will be great bikeshedding about division of our current flat namespace, I certainly hope that day has not come yet, though ;-) > > 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 Great! > > > 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 > ). That rule is a very good one and I think all the worries apply very well to getter methods too. (Although, really real C++ projects have both getters and const getters, I think, so in a really real C++ code your getter would be a const getter anyway ;-). Moreover, I think we should be extra careful here because GCC is transitioning from C and many developers are used to expect C semantics, we will be mixing new C++ code with C code a lot in the future and even though we hope to reduce that, people will be cutting and pasting all this stuff around. > > 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'm not afraid of bugs when we dereference a NULL, those are easy to fix. On the other hand I am afraid of bugs caused by inadvertent modification of data that is hard to spot, those can be horrible to debug. > > 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? We already use them in vectors (and they have already confused me there although I don't remember how exactly - but it was a syntax problem). I personally do not really like them and consider them a bit alien. However, I'm not really against const references where it can save some typing or even against non-const ones when they are necessary (but the only such situation I can think of is operator overloading). > > There is another question here, which is how people feel about > accessors/getters? I think people generally like them, except when they are stepping through multitudes of them in gdb. There is a tradeoff but I think that at the moment people are expected to add them for interfaces with wider use (and can choose to avoid them when interface use is not so wide, e.g. restricted to a particular pass). > 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? IIRC, it was anonymous before so it's safe to say nobody does, it is just there to define the enum elements (to be used as integers?), I suppose. Thanks, Martin
