Hi,
I don't know why it's me again but again I do have a few comments.
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?
A few more comments inline:
On Wed, Jul 24, 2013 at 11:09:04AM -0400, David Malcolm wrote:
> gcc/
> * Makefile.in (PIPELINE_H): New.
> (lto-cgraph.o): Depend on CONTEXT_H and PIPELINE_H.
> (passes.o): Likewise.
> (statistics.o): Likewise.
> (cgraphunit.o): Likewise.
> (context.o): Depend on PIPELINE_H.
>
> * pipeline.h: New.
>
> * cgraphunit.c (cgraph_add_new_function): Update for moves
> of globals to fields of pipeline.
> (analyze_function): Likewise.
> (expand_function): Likewise.
> (ipa_passes): Likewise.
> (compile): Likewise.
>
> * context.c (context::context): New.
> * context.h (context::context): New.
> (context::get_passes): New.
> (context::passes_): New.
>
> * lto-cgraph.c (input_node): Update for moves of globals to
> fields of pipeline.
>
> * passes.c (all_passes): Remove, in favor of a field of the
> same name within the new class pipeline.
> (all_small_ipa_passes): Likewise.
> (all_lowering_passes): Likewise.
> (all_regular_ipa_passes): Likewise.
> (all_late_ipa_passes): Likewise.
> (all_lto_gen_passes): Likewise.
> (passes_by_id): Likewise.
> (passes_by_id_size): Likewise.
> (gcc_pass_lists): Remove, in favor of "pass_lists" field within
> the new class pipeline.
> (set_pass_for_id): Convert to...
> (pipeline::set_pass_for_id): ...method.
> (get_pass_for_id): Convert to...
> (pipeline::get_pass_for_id): ...method.
> (register_one_dump_file): Move body of implementation into...
> (pipeline::register_one_dump_file): ...here.
> (register_dump_files_1): Convert to...
> (pipeline::register_dump_files_1): ...method.
> (register_dump_files): Convert to...
> (pipeline::register_dump_files): ...method.
> (create_pass_tab): Update for moves of globals to fields of
> pipeline.
> (dump_passes): Move body of implementation into...
> (pipeline::dump_passes): ...here.
> (register_pass): Move body of implementation into...
> (pipeline::register_pass): ...here.
> (init_optimization_passes): Convert into...
> (pipeline::pipeline): ...constructor for new pipeline class, and
> initialize the pass_lists array.
> (check_profile_consistency): Update for moves of globals to
> fields of pipeline.
> (dump_profile_report): Move body of implementation into...
> (pipeline::dump_profile_report): ...here.
> (ipa_write_summaries_1): Update for moves of pass lists from
> being globals to fields of pipeline.
> (ipa_write_optimization_summaries): Likewise.
> (ipa_read_summaries): Likewise.
> (ipa_read_optimization_summaries): Likewise.
> (execute_all_ipa_stmt_fixups): Likewise.
>
> * statistics.c (statistics_fini): Update for moves of globals to
> fields of pipeline.
>
> * toplev.c (general_init): Replace call to
> init_optimization_passes with construction of the pipeline
> instance.
>
> * tree-pass.h (all_passes): Remove, in favor of a field of the
> same name within the new class pipeline.
> (all_small_ipa_passes): Likewise.
> (all_lowering_passes): Likewise.
> (all_regular_ipa_passes): Likewise.
> (all_lto_gen_passes): Likewise.
> (all_late_ipa_passes): Likewise.
> (passes_by_id): Likewise.
> (passes_by_id_size): Likewise.
> (gcc_pass_lists): Remove, in favor of "pass_lists" field within
> the new class pipeline.
> (get_pass_for_id): Remove.
>
> gcc/lto/
>
> * Make-lang.in (lto/lto.o:): Depend on CONTEXT_H and
> PIPELINE_H.
>
> * lto.c (do_whole_program_analysis): Update for move of
> all_regular_ipa_passes from a global to a field of class
> pipeline.
...
> 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 :-)
>
> 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...)
> 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.)
>
> - /* 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.
> tree fn_decl;
> struct cgraph_node *node;
> struct bitpack_d bp;
> @@ -981,8 +984,8 @@ input_node (struct lto_file_decl_data *file_data,
> struct opt_pass *pass;
> int pid = streamer_read_hwi (ib);
>
> - gcc_assert (pid < passes_by_id_size);
> - pass = passes_by_id[pid];
> + gcc_assert (pid < passes.passes_by_id_size);
> + pass = passes.passes_by_id[pid];
> node->ipa_transforms_to_apply.safe_push ((struct ipa_opt_pass_d *)
> pass);
> }
>
...
> 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?
> +{
> + 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?
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 :-)
Martin
