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

[David Malcolm]

On Thu, 2013-10-10 at 09:56 +0200, Richard Biener wrote:
> 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?

(sorry about the belated response)

The reason for the "has_foo_member_function" flags is that everywhere we
use them, the style is more like this:

if (pass_has_foo_member_function)
  {
     do_various_stuff ();
     pass->foo_member_function ();
     do_more_stuff ();
  }

where the precise "stuff" varies between the different vfuncs.  I don't
think there's a portable way to check if a vfunc has been overridden, so
having these flags seems the best way of handling this kind of logic.

It's very similar to what we now have for the "gate" and "execute" hooks
for the opt_pass base class.   (In the first version of that work that I
posted, I had vfuncs for the "has_" logic, but simple bool flags let us
avoid that virtual call, hence I went with the latter approach).

The patches still apply cleanly to trunk; I'm rechecking the bootstrap
and regtest.  Are they OK for trunk if the testing passes?
[For context: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00639.html ]

Thanks
Dave

> 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
> >