Re: [PATCH 1/2] Handwritten part of conversion of IPA pass hooks to virtual functions

[Richard Biener <richard dot guenther at gmail dot com>]

As a general observation I don't like the

 if (pass->has_foo_member_function)
   pass->foo_member_function ()

style.  It is totally against the idea of having virtual functions.  Why
not simply provide stub implementations in the base classes?

Richard.

On Thu, Oct 10, 2013 at 2:41 AM, David Malcolm <dmalcolm@redhat.com> wrote:
>         * passes.c (ipa_opt_pass_d::generate_summary): New.
>         (ipa_opt_pass_d::write_summary): New.
>         (ipa_opt_pass_d::read_summary): New.
>         (ipa_opt_pass_d::write_optimization_summary): New.
>         (ipa_opt_pass_d::read_optimization_summary): New.
>         (ipa_opt_pass_d::stmt_fixup): New.
>         (ipa_opt_pass_d::function_transform): New.
>         (ipa_opt_pass_d::variable_transform): New.
>         (execute_ipa_summary_passes): Use ipa_data_. to determine if
>         pass has a generate_summary implemenation, since we can't check
>         a vtable entry.
>         (execute_one_ipa_transform_pass): Likewise for
>         function_transform, and for the
>         function_transform_todo_flags_start field.
>         (ipa_write_summaries_2): Likewise for write_summary.
>         (ipa_write_optimization_summaries_1): Likewise for
>         write_optimization_summary.
>         (ipa_read_summaries_1): Likewise for read_summary.
>         (ipa_read_optimization_summaries_1): Likewise for
>         read_optimization_summary.
>         (execute_ipa_stmt_fixups): Likewise for stmt_fixup.
>
>         * tree-pass.h (struct ipa_pass_data): New.
>         (ipa_opt_pass_d::generate_summary): Convert to virtual function.
>         (ipa_opt_pass_d::write_summary): Likewise.
>         (ipa_opt_pass_d::read_summary): Likewise.
>         (ipa_opt_pass_d::write_optimization_summary): Likewise.
>         (ipa_opt_pass_d::read_optimization_summary): Likewise.
>         (ipa_opt_pass_d::stmt_fixup): Likewise.
>         (ipa_opt_pass_d::function_transform_todo_flags_start): Drop;
>         this is now part of ipa_pass_data.
>         (ipa_opt_pass_d::function_transform): Convert to virtual function.
>         (ipa_opt_pass_d::variable_transform): Likewise.
>         (ipa_opt_pass_d::ipa_data_): New.
>         (ipa_opt_pass_d::ipa_opt_pass_d): Drop function pointer fields in
>         favor of virtual functions and an ipa_pass_data.
> ---
>  gcc/passes.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  gcc/tree-pass.h | 54 ++++++++++++++++++++++----------------------
>  2 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 1b2202e..625f1d3 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -117,7 +117,59 @@ opt_pass::opt_pass (const pass_data &data, context *ctxt)
>  {
>  }
>
> +/* Default implementations of the various ipa_opt_pass_d hooks,
> +   provided to detect inconsistent pass metadata.
>
> +   These implementations should never be called: any pass that sets a
> +   "has_" flag it its ipa_pass_data should provide a function overriding
> +   the corresponding default implementation.  */
> +void
> +ipa_opt_pass_d::generate_summary (void)
> +{
> +  internal_error ("base generate_summary called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::write_summary (void)
> +{
> +  internal_error ("base write_summary called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::read_summary (void)
> +{
> +  internal_error ("base read_summary called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::write_optimization_summary (void)
> +{
> +  internal_error ("base write_optimization_summary called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::read_optimization_summary (void)
> +{
> +  internal_error ("base read_optimization_summary called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::stmt_fixup (struct cgraph_node *, gimple *)
> +{
> +  internal_error ("base stmt_fixup called for %s", name);
> +}
> +
> +unsigned int
> +ipa_opt_pass_d::function_transform (struct cgraph_node *)
> +{
> +  internal_error ("base function_transform called for %s", name);
> +}
> +
> +void
> +ipa_opt_pass_d::variable_transform (struct varpool_node *)
> +{
> +  internal_error ("base variable_transform called for %s", name);
> +}
>  void
>  pass_manager::execute_early_local_passes ()
>  {
> @@ -1990,7 +2042,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
>        /* Execute all of the IPA_PASSes in the list.  */
>        if (ipa_pass->type == IPA_PASS
>           && ((!pass->has_gate) || pass->gate ())
> -         && ipa_pass->generate_summary)
> +         && ipa_pass->m_ipa_data.has_generate_summary)
>         {
>           pass_init_dump_file (pass);
>
> @@ -2020,7 +2072,7 @@ execute_one_ipa_transform_pass (struct cgraph_node *node,
>    unsigned int todo_after = 0;
>
>    current_pass = pass;
> -  if (!ipa_pass->function_transform)
> +  if (!ipa_pass->m_ipa_data.has_function_transform)
>      return;
>
>    /* Note that the folders should only create gimple expressions.
> @@ -2030,7 +2082,7 @@ execute_one_ipa_transform_pass (struct cgraph_node *node,
>    pass_init_dump_file (pass);
>
>    /* Run pre-pass verification.  */
> -  execute_todo (ipa_pass->function_transform_todo_flags_start);
> +  execute_todo (ipa_pass->m_ipa_data.function_transform_todo_flags_start);
>
>    /* If a timevar is present, start it.  */
>    if (pass->tv_id != TV_NONE)
> @@ -2272,7 +2324,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
>        gcc_assert (!cfun);
>        gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>        if (pass->type == IPA_PASS
> -         && ipa_pass->write_summary
> +         && ipa_pass->m_ipa_data.has_write_summary
>           && ((!pass->has_gate) || pass->gate ()))
>         {
>           /* If a timevar is present, start it.  */
> @@ -2388,7 +2440,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
>        gcc_assert (!cfun);
>        gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>        if (pass->type == IPA_PASS
> -         && ipa_pass->write_optimization_summary
> +         && ipa_pass->m_ipa_data.has_write_optimization_summary
>           && ((!pass->has_gate) || pass->gate ()))
>         {
>           /* If a timevar is present, start it.  */
> @@ -2468,7 +2520,8 @@ ipa_read_summaries_1 (struct opt_pass *pass)
>
>        if ((!pass->has_gate) || pass->gate ())
>         {
> -         if (pass->type == IPA_PASS && ipa_pass->read_summary)
> +         if (pass->type == IPA_PASS
> +             && ipa_pass->m_ipa_data.has_read_summary)
>             {
>               /* If a timevar is present, start it.  */
>               if (pass->tv_id)
> @@ -2519,7 +2572,8 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
>
>        if ((!pass->has_gate) || pass->gate ())
>         {
> -         if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> +         if (pass->type == IPA_PASS
> +             && ipa_pass->m_ipa_data.has_read_optimization_summary)
>             {
>               /* If a timevar is present, start it.  */
>               if (pass->tv_id)
> @@ -2599,7 +2653,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
>         {
>           struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
>
> -         if (ipa_pass->stmt_fixup)
> +         if (ipa_pass->m_ipa_data.has_stmt_fixup)
>             {
>               pass_init_dump_file (pass);
>               /* If a timevar is present, start it.  */
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index e72fe9a..5f323dc 100644
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -69,6 +69,20 @@ struct pass_data
>    unsigned int todo_flags_finish;
>  };
>
> +/* Additional metadata for IPA_PASS.  */
> +struct ipa_pass_data
> +{
> +  bool has_generate_summary;
> +  bool has_write_summary;
> +  bool has_read_summary;
> +  bool has_write_optimization_summary;
> +  bool has_read_optimization_summary;
> +  bool has_stmt_fixup;
> +  unsigned int function_transform_todo_flags_start;
> +  bool has_function_transform;
> +  bool has_variable_transform;
> +};
> +
>  namespace gcc
>  {
>    class context;
> @@ -149,51 +163,37 @@ class ipa_opt_pass_d : public opt_pass
>  public:
>    /* IPA passes can analyze function body and variable initializers
>        using this hook and produce summary.  */
> -  void (*generate_summary) (void);
> +  virtual void generate_summary (void);
>
>    /* This hook is used to serialize IPA summaries on disk.  */
> -  void (*write_summary) (void);
> +  virtual void write_summary (void);
>
>    /* This hook is used to deserialize IPA summaries from disk.  */
> -  void (*read_summary) (void);
> +  virtual void read_summary (void);
>
>    /* This hook is used to serialize IPA optimization summaries on disk.  */
> -  void (*write_optimization_summary) (void);
> +  virtual void write_optimization_summary (void);
>
>    /* This hook is used to deserialize IPA summaries from disk.  */
> -  void (*read_optimization_summary) (void);
> +  virtual void read_optimization_summary (void);
>
>    /* 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 void stmt_fixup (struct cgraph_node *, gimple *);
>
>    /* 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 *);
> +  virtual unsigned int function_transform (struct cgraph_node *);
> +  virtual void variable_transform (struct varpool_node *);
> +
> +public:
> +  const ipa_pass_data &m_ipa_data;
>
>  protected:
>    ipa_opt_pass_d (const pass_data& data, gcc::context *ctxt,
> -                 void (*generate_summary) (void),
> -                 void (*write_summary) (void),
> -                 void (*read_summary) (void),
> -                 void (*write_optimization_summary) (void),
> -                 void (*read_optimization_summary) (void),
> -                 void (*stmt_fixup) (struct cgraph_node *, gimple *),
> -                 unsigned int function_transform_todo_flags_start,
> -                 unsigned int (*function_transform) (struct cgraph_node *),
> -                 void (*variable_transform) (struct varpool_node *))
> +                 const ipa_pass_data &ipa_data)
>      : opt_pass (data, ctxt),
> -      generate_summary (generate_summary),
> -      write_summary (write_summary),
> -      read_summary (read_summary),
> -      write_optimization_summary (write_optimization_summary),
> -      read_optimization_summary (read_optimization_summary),
> -      stmt_fixup (stmt_fixup),
> -      function_transform_todo_flags_start (function_transform_todo_flags_start),
> -      function_transform (function_transform),
> -      variable_transform (variable_transform)
> +      m_ipa_data (ipa_data)
>    {
>    }
>  };
> --
> 1.7.11.7
>