[1/2] handwritten part of patch

Eliminate all direct references to "cfun" from macros in
	basic-block.h and introduce access methods to control_flow_graph

	* basic-block.h (control_flow_graph::get_basic_block_by_idx): New
	accessor methods.
	(control_flow_graph::get_entry_block): New method.
	(control_flow_graph::get_exit_block): New method.
	(control_flow_graph::get_basic_block_info): New methods.
	(control_flow_graph::get_n_basic_blocks): New methods.
	(control_flow_graph::get_n_edges): New methods.
	(control_flow_graph::get_last_basic_block): New methods.
	(control_flow_graph::get_label_to_block_map): New methods.
	(control_flow_graph::get_profile_status): New method.
	(control_flow_graph::set_profile_status): New method.
	(ENTRY_BLOCK_PTR): Eliminate this macro.
	(EXIT_BLOCK_PTR): Likewise.
	(basic_block_info): Likewise.
	(n_basic_blocks): Likewise.
	(n_edges): Likewise.
	(last_basic_block): Likewise.
	(label_to_block_map): Likewise.
	(profile_status): Likewise.
	(BASIC_BLOCK): Likewise.
	(SET_BASIC_BLOCK): Likewise.
	(FOR_EACH_BB_FN): Rewrite in terms of...
	(FOR_EACH_BB_CFG): New macro
	(FOR_EACH_BB): Eliminate this macro
	(FOR_EACH_BB_REVERSE_FN): Rewrite in terms of...
	(FOR_EACH_BB_REVERSE_FN_CFG): New macro
	(FOR_EACH_BB_REVERSE): Eliminate this macro
	(FOR_ALL_BB): Likewise.
	(FOR_ALL_BB_CFG): New macro

---

On Sat, May 25, 2013 at 3:02 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>         Eliminate all direct references to "cfun" from macros in
>         basic-block.h and introduce access methods to control_flow_graph
>
>         * basic-block.h (control_flow_graph::get_basic_block_by_idx): New
>         accessor methods.
>         (control_flow_graph::get_entry_block): New method.
>         (control_flow_graph::get_exit_block): New method.
>         (control_flow_graph::get_basic_block_info): New methods.
>         (control_flow_graph::get_n_basic_blocks): New methods.
>         (control_flow_graph::get_n_edges): New methods.
>         (control_flow_graph::get_last_basic_block): New methods.
>         (control_flow_graph::get_label_to_block_map): New methods.
>         (control_flow_graph::get_profile_status): New method.
>         (control_flow_graph::set_profile_status): New method.
>         (ENTRY_BLOCK_PTR): Eliminate this macro.
>         (EXIT_BLOCK_PTR): Likewise.
>         (basic_block_info): Likewise.
>         (n_basic_blocks): Likewise.
>         (n_edges): Likewise.
>         (last_basic_block): Likewise.
>         (label_to_block_map): Likewise.
>         (profile_status): Likewise.
>         (BASIC_BLOCK): Likewise.
>         (SET_BASIC_BLOCK): Likewise.
>         (FOR_EACH_BB_FN): Rewrite in terms of...
>         (FOR_EACH_BB_CFG): New macro
>         (FOR_EACH_BB): Eliminate this macro
>         (FOR_EACH_BB_REVERSE_FN): Rewrite in terms of...
>         (FOR_EACH_BB_REVERSE_FN_CFG): New macro
>         (FOR_EACH_BB_REVERSE): Eliminate this macro
>         (FOR_ALL_BB): Likewise.
>         (FOR_ALL_BB_CFG): New macro

I don't like the mix of _CFG / _FN.  It's obvious we are talking about
the CFG of a function in 'FOR_EACH_BB' and friends.

> ---
> diff --git a/gcc/basic-block.h b/gcc/basic-block.h
> index eed320c..3949417 100644
> --- a/gcc/basic-block.h
> +++ b/gcc/basic-block.h
> @@ -276,6 +276,57 @@ enum profile_status_d
>     fields of this struct are interpreted as the defines for backward
>     source compatibility following the definition of this struct.  */
>  struct GTY(()) control_flow_graph {
> +public:
> +  basic_block get_basic_block_by_idx (int idx) const
> +  {
> +    return (*x_basic_block_info)[idx];
> +  }

get_basic_block_by_idx is rather long, any reason to not shorten
it to get_block () or get_bb?

> +  void set_basic_block_by_idx (int idx, basic_block bb)

set_block () or set_bb()

> +  {
> +    (*x_basic_block_info)[idx] = bb;
> +  }
> +
> +  basic_block get_entry_block () const { return x_entry_block_ptr; }
> +
> +  basic_block get_exit_block () const { return x_exit_block_ptr; }
> +
> +  vec<basic_block, va_gc> *get_basic_block_info () const

are they really used outside of the iterators?

> +  {
> +    return x_basic_block_info;
> +  }
> +  vec<basic_block, va_gc> *&get_basic_block_info ()
> +  {
> +    return x_basic_block_info;
> +  }
> +
> +  int get_n_basic_blocks () const { return x_n_basic_blocks; }
> +  int& get_n_basic_blocks () { return x_n_basic_blocks; }
> +
> +  int get_n_edges () const { return x_n_edges; }
> +  int& get_n_edges () { return x_n_edges; }
> +
> +  int get_last_basic_block () const { return x_last_basic_block; }
> +  int& get_last_basic_block () { return x_last_basic_block; }

As you can't set_ those I'd drop the get_.  Any reason to not simply
drop the x_ in the members and allow direct accesses?  I fear
an -O0 compiler will get even more un-debuggable that it is now
with all the accessors :/ (undebuggable as in stepping through source)

> +  vec<basic_block, va_gc> *get_label_to_block_map () const
> +  {
> +    return x_label_to_block_map;
> +  }
> +  vec<basic_block, va_gc> *&get_label_to_block_map ()
> +  {
> +    return x_label_to_block_map;
> +  }
> +
> +  enum profile_status_d get_profile_status () const
> +  {
> +    return x_profile_status;
> +  }
> +  void set_profile_status (enum profile_status_d status)
> +  {
> +    x_profile_status = status;
> +  }
> +
> +public:
>    /* Block pointers for the exit and entry of a function.
>       These are always the head and tail of the basic block list.  */
>    basic_block x_entry_block_ptr;
> @@ -328,32 +379,20 @@ struct GTY(()) control_flow_graph {
>  #define SET_BASIC_BLOCK_FOR_FUNCTION(FN,N,BB) \
>    ((*basic_block_info_for_function(FN))[(N)] = (BB))
>
> -/* Defines for textual backward source compatibility.  */
> -#define ENTRY_BLOCK_PTR                (cfun->cfg->x_entry_block_ptr)
> -#define EXIT_BLOCK_PTR         (cfun->cfg->x_exit_block_ptr)
> -#define basic_block_info       (cfun->cfg->x_basic_block_info)
> -#define n_basic_blocks         (cfun->cfg->x_n_basic_blocks)
> -#define n_edges                        (cfun->cfg->x_n_edges)
> -#define last_basic_block       (cfun->cfg->x_last_basic_block)
> -#define label_to_block_map     (cfun->cfg->x_label_to_block_map)
> -#define profile_status         (cfun->cfg->x_profile_status)
> -
> -#define BASIC_BLOCK(N)         ((*basic_block_info)[(N)])
> -#define SET_BASIC_BLOCK(N,BB)  ((*basic_block_info)[(N)] = (BB))
> -

Replacements with the macros that already exist and expose cfun would
have been an obvious patch btw, without trying to refactor things to look
like C++ (ugh).

Richard.

>  /* For iterating over basic blocks.  */
>  #define FOR_BB_BETWEEN(BB, FROM, TO, DIR) \
>    for (BB = FROM; BB != TO; BB = BB->DIR)
>
> -#define FOR_EACH_BB_FN(BB, FN) \
> -  FOR_BB_BETWEEN (BB, (FN)->cfg->x_entry_block_ptr->next_bb, (FN)->cfg->x_exit_block_ptr, next_bb)
> +#define FOR_EACH_BB_CFG(BB, CFG) \
> +  FOR_BB_BETWEEN (BB, (CFG)->x_entry_block_ptr->next_bb, (CFG)->x_exit_block_ptr, next_bb)
>
> -#define FOR_EACH_BB(BB) FOR_EACH_BB_FN (BB, cfun)
> +#define FOR_EACH_BB_FN(BB, FN) FOR_EACH_BB_CFG (BB, (FN)->cfg)
>
> -#define FOR_EACH_BB_REVERSE_FN(BB, FN) \
> -  FOR_BB_BETWEEN (BB, (FN)->cfg->x_exit_block_ptr->prev_bb, (FN)->cfg->x_entry_block_ptr, prev_bb)
> +#define FOR_EACH_BB_REVERSE_CFG(BB, CFG) \
> +  FOR_BB_BETWEEN (BB, (CFG)->x_exit_block_ptr->prev_bb, (CFG)->x_entry_block_ptr, prev_bb)
>
> -#define FOR_EACH_BB_REVERSE(BB) FOR_EACH_BB_REVERSE_FN(BB, cfun)
> +#define FOR_EACH_BB_REVERSE_FN(BB, FN) \
> +  FOR_EACH_BB_REVERSE_FN_CFG (BB, (FN)->cfg)
>
>  /* For iterating over insns in basic block.  */
>  #define FOR_BB_INSNS(BB, INSN)                 \
> @@ -380,9 +419,8 @@ struct GTY(()) control_flow_graph {
>
>  /* Cycles through _all_ basic blocks, even the fake ones (entry and
>     exit block).  */
> -
> -#define FOR_ALL_BB(BB) \
> -  for (BB = ENTRY_BLOCK_PTR; BB; BB = BB->next_bb)
> +#define FOR_ALL_BB_CFG(BB, CFG) \
> +  for (BB = (CFG)->get_entry_block (); BB; BB = BB->next_bb)
>
>  #define FOR_ALL_BB_FN(BB, FN) \
>    for (BB = ENTRY_BLOCK_PTR_FOR_FUNCTION (FN); BB; BB = BB->next_bb)
> --
> 1.7.11.7
>

On Mon, 2013-05-27 at 15:38 +0200, Richard Biener wrote:
> On Sat, May 25, 2013 at 3:02 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> >         Eliminate all direct references to "cfun" from macros in
> >         basic-block.h and introduce access methods to control_flow_graph
> >
> >         * basic-block.h (control_flow_graph::get_basic_block_by_idx): New
> >         accessor methods.
> >         (control_flow_graph::get_entry_block): New method.
> >         (control_flow_graph::get_exit_block): New method.
> >         (control_flow_graph::get_basic_block_info): New methods.
> >         (control_flow_graph::get_n_basic_blocks): New methods.
> >         (control_flow_graph::get_n_edges): New methods.
> >         (control_flow_graph::get_last_basic_block): New methods.
> >         (control_flow_graph::get_label_to_block_map): New methods.
> >         (control_flow_graph::get_profile_status): New method.
> >         (control_flow_graph::set_profile_status): New method.
> >         (ENTRY_BLOCK_PTR): Eliminate this macro.
> >         (EXIT_BLOCK_PTR): Likewise.
> >         (basic_block_info): Likewise.
> >         (n_basic_blocks): Likewise.
> >         (n_edges): Likewise.
> >         (last_basic_block): Likewise.
> >         (label_to_block_map): Likewise.
> >         (profile_status): Likewise.
> >         (BASIC_BLOCK): Likewise.
> >         (SET_BASIC_BLOCK): Likewise.
> >         (FOR_EACH_BB_FN): Rewrite in terms of...
> >         (FOR_EACH_BB_CFG): New macro
> >         (FOR_EACH_BB): Eliminate this macro
> >         (FOR_EACH_BB_REVERSE_FN): Rewrite in terms of...
> >         (FOR_EACH_BB_REVERSE_FN_CFG): New macro
> >         (FOR_EACH_BB_REVERSE): Eliminate this macro
> >         (FOR_ALL_BB): Likewise.
> >         (FOR_ALL_BB_CFG): New macro
> 
> I don't like the mix of _CFG / _FN.  It's obvious we are talking about
> the CFG of a function in 'FOR_EACH_BB' and friends.

The current status quo is a pair of macros:

  #define FOO_FN(FN) ...do something on (FN)->cfg
  #define FOO() FOO_FN(cfun)

My patch changed these to be:

  #define FOO_CFG(CFG) ...do something on CFG
  #define FOO_FN(FN) FOO_CFG((FN)->cfg)

and to get rid of the FOO() with its cfun usage.

So would your preference be for the FOO() to mean the cfg:

  #define FOO(CFG) ...do something on cfg
  #define FOO_FN(FN) FOO((FN)->cfg)

?

e.g. 

#define FOR_EACH_BB(BB, CFG) \
  FOR_BB_BETWEEN (BB, (CFG)->x_entry_block_ptr->next_bb, (CFG)->x_exit_block_ptr, next_bb)

#define FOR_EACH_BB_FN(BB, FN) FOR_EACH_BB_CFG (BB, (FN)->cfg)


> > ---
> > diff --git a/gcc/basic-block.h b/gcc/basic-block.h
> > index eed320c..3949417 100644
> > --- a/gcc/basic-block.h
> > +++ b/gcc/basic-block.h
> > @@ -276,6 +276,57 @@ enum profile_status_d
> >     fields of this struct are interpreted as the defines for backward
> >     source compatibility following the definition of this struct.  */
> >  struct GTY(()) control_flow_graph {
> > +public:
> > +  basic_block get_basic_block_by_idx (int idx) const
> > +  {
> > +    return (*x_basic_block_info)[idx];
> > +  }
> 
> get_basic_block_by_idx is rather long, any reason to not shorten
> it to get_block () or get_bb?

Will do.   get_bb () then

> > +  void set_basic_block_by_idx (int idx, basic_block bb)
> 
> set_block () or set_bb()

Will do.

> > +  {
> > +    (*x_basic_block_info)[idx] = bb;
> > +  }
> > +
> > +  basic_block get_entry_block () const { return x_entry_block_ptr; }
> > +
> > +  basic_block get_exit_block () const { return x_exit_block_ptr; }
> > +
> > +  vec<basic_block, va_gc> *get_basic_block_info () const
> 
> are they really used outside of the iterators?

The {entry|exit}_block are used in many places, which appears to almost
all be comparisons of a basic_block to entry/exit.

The get_basic_block_info () is used in just 3 places due to:

  if (basic_block_info->length () < SOME_VALUE) 
    vec_safe_grow_cleared (basic_block_info, SOME_OTHER_VALUE); 

so perhaps that's worth abstracting.
(specifically, in:
  * gcc/cfgrtl.c: rtl_create_basic_block
  * gcc/tree-cfg.c: build_gimple_cfg
  * gcc/tree-cfg.c: create_bb
)


> > +  {
> > +    return x_basic_block_info;
> > +  }
> > +  vec<basic_block, va_gc> *&get_basic_block_info ()
> > +  {
> > +    return x_basic_block_info;
> > +  }
> > +
> > +  int get_n_basic_blocks () const { return x_n_basic_blocks; }
> > +  int& get_n_basic_blocks () { return x_n_basic_blocks; }
> > +
> > +  int get_n_edges () const { return x_n_edges; }
> > +  int& get_n_edges () { return x_n_edges; }
> > +
> > +  int get_last_basic_block () const { return x_last_basic_block; }
> > +  int& get_last_basic_block () { return x_last_basic_block; }
> 
> As you can't set_ those I'd drop the get_.  Any reason to not simply
> drop the x_ in the members and allow direct accesses?  I fear
> an -O0 compiler will get even more un-debuggable that it is now
> with all the accessors :/ (undebuggable as in stepping through source)

My thinking here was to make the x_ things be private, to make it easier
to grep (or breakpoint) where changes happen.

But it sounds like you'd prefer to simply drop the x_ prefixes and have
direct access, so sure.

> > +  vec<basic_block, va_gc> *get_label_to_block_map () const
> > +  {
> > +    return x_label_to_block_map;
> > +  }
> > +  vec<basic_block, va_gc> *&get_label_to_block_map ()
> > +  {
> > +    return x_label_to_block_map;
> > +  }
> > +
> > +  enum profile_status_d get_profile_status () const
> > +  {
> > +    return x_profile_status;
> > +  }
> > +  void set_profile_status (enum profile_status_d status)
> > +  {
> > +    x_profile_status = status;
> > +  }
> > +
> > +public:
> >    /* Block pointers for the exit and entry of a function.
> >       These are always the head and tail of the basic block list.  */
> >    basic_block x_entry_block_ptr;
> > @@ -328,32 +379,20 @@ struct GTY(()) control_flow_graph {
> >  #define SET_BASIC_BLOCK_FOR_FUNCTION(FN,N,BB) \
> >    ((*basic_block_info_for_function(FN))[(N)] = (BB))
> >
> > -/* Defines for textual backward source compatibility.  */
> > -#define ENTRY_BLOCK_PTR                (cfun->cfg->x_entry_block_ptr)
> > -#define EXIT_BLOCK_PTR         (cfun->cfg->x_exit_block_ptr)
> > -#define basic_block_info       (cfun->cfg->x_basic_block_info)
> > -#define n_basic_blocks         (cfun->cfg->x_n_basic_blocks)
> > -#define n_edges                        (cfun->cfg->x_n_edges)
> > -#define last_basic_block       (cfun->cfg->x_last_basic_block)
> > -#define label_to_block_map     (cfun->cfg->x_label_to_block_map)
> > -#define profile_status         (cfun->cfg->x_profile_status)
> > -
> > -#define BASIC_BLOCK(N)         ((*basic_block_info)[(N)])
> > -#define SET_BASIC_BLOCK(N,BB)  ((*basic_block_info)[(N)] = (BB))
> > -
> 
> Replacements with the macros that already exist and expose cfun would
> have been an obvious patch btw, without trying to refactor things to look
> like C++ (ugh).

I'd like to make the cfun->cfg dereference explicit if possible, since
we may want to optimize the repeated derefs away - we know (but the
compiler doesn't) that cfun->cfg doesn't change.

So I'll drop the excessive C++-isms, and try another version of this,
which gets rid of the x_ prefixes, and where the CFG is passed in as the
macro param, if that sounds reasonable.  (it sounded like the get_bb ()
method was acceptable)

Thanks for your comments
Dave

On Tue, May 28, 2013 at 8:04 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Mon, 2013-05-27 at 15:38 +0200, Richard Biener wrote:
>> On Sat, May 25, 2013 at 3:02 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> >         Eliminate all direct references to "cfun" from macros in
>> >         basic-block.h and introduce access methods to control_flow_graph
>> >
>> >         * basic-block.h (control_flow_graph::get_basic_block_by_idx): New
>> >         accessor methods.
>> >         (control_flow_graph::get_entry_block): New method.
>> >         (control_flow_graph::get_exit_block): New method.
>> >         (control_flow_graph::get_basic_block_info): New methods.
>> >         (control_flow_graph::get_n_basic_blocks): New methods.
>> >         (control_flow_graph::get_n_edges): New methods.
>> >         (control_flow_graph::get_last_basic_block): New methods.
>> >         (control_flow_graph::get_label_to_block_map): New methods.
>> >         (control_flow_graph::get_profile_status): New method.
>> >         (control_flow_graph::set_profile_status): New method.
>> >         (ENTRY_BLOCK_PTR): Eliminate this macro.
>> >         (EXIT_BLOCK_PTR): Likewise.
>> >         (basic_block_info): Likewise.
>> >         (n_basic_blocks): Likewise.
>> >         (n_edges): Likewise.
>> >         (last_basic_block): Likewise.
>> >         (label_to_block_map): Likewise.
>> >         (profile_status): Likewise.
>> >         (BASIC_BLOCK): Likewise.
>> >         (SET_BASIC_BLOCK): Likewise.
>> >         (FOR_EACH_BB_FN): Rewrite in terms of...
>> >         (FOR_EACH_BB_CFG): New macro
>> >         (FOR_EACH_BB): Eliminate this macro
>> >         (FOR_EACH_BB_REVERSE_FN): Rewrite in terms of...
>> >         (FOR_EACH_BB_REVERSE_FN_CFG): New macro
>> >         (FOR_EACH_BB_REVERSE): Eliminate this macro
>> >         (FOR_ALL_BB): Likewise.
>> >         (FOR_ALL_BB_CFG): New macro
>>
>> I don't like the mix of _CFG / _FN.  It's obvious we are talking about
>> the CFG of a function in 'FOR_EACH_BB' and friends.
>
> The current status quo is a pair of macros:
>
>   #define FOO_FN(FN) ...do something on (FN)->cfg
>   #define FOO() FOO_FN(cfun)
>
> My patch changed these to be:
>
>   #define FOO_CFG(CFG) ...do something on CFG
>   #define FOO_FN(FN) FOO_CFG((FN)->cfg)
>
> and to get rid of the FOO() with its cfun usage.
>
> So would your preference be for the FOO() to mean the cfg:
>
>   #define FOO(CFG) ...do something on cfg
>   #define FOO_FN(FN) FOO((FN)->cfg)
>
> ?
>
> e.g.
>
> #define FOR_EACH_BB(BB, CFG) \
>   FOR_BB_BETWEEN (BB, (CFG)->x_entry_block_ptr->next_bb, (CFG)->x_exit_block_ptr, next_bb)
>
> #define FOR_EACH_BB_FN(BB, FN) FOR_EACH_BB_CFG (BB, (FN)->cfg)

I suppose the CFG vs. FN distinction is because there is a
control-flow-graph sub-structure in a function (OTOH there is at most
a single CFG in a FN, so specifying FN is unambiguous).

Yes, the un-suffixed variant (FOO) should be the one with the "natural"
interface (taking a CFG).  People already know about the _FN variant
and if you change all existing uses of FOO (without cfg/fn argument)
then that would be the best change.

Ideally FN or CFG would be handled transparently via overloading
for example (or giving both struct function and struct cfg the same
set of member functions), so

inline basic_block entry_block_ptr (struct function *fn)
{
  return fn->cfg->x_entry_block_ptr;
}
inline basic_block entry_block_ptr (struct cfg *)
{
  return cfg->x_entry_block_ptr;
}

#define FOR_EACH_BB (BB, CONTEXT) FOR_BB_BETWEEN (BB, entry_block_ptr
(CONTEXT)->next_bb, exit_block_ptr (CONTEXT), next_bb)

and only retain a single macro/function form for context kinds that can
be unambiguously interchanged.

Or do people think that would be too confusing?  (I suppose defining an
automatic conversion from struct function * to struct cfg * is also techincally
possible, but that only works with the non-member-function way and would
avoid the overloading).

That said, I have no strong preference to remove the FOO_FN variants together
with this patch.  Using FOO with cfg arguments and FOO_FN with function
arguments is fine for me (eventually as intermediate step).

>> > ---
>> > diff --git a/gcc/basic-block.h b/gcc/basic-block.h
>> > index eed320c..3949417 100644
>> > --- a/gcc/basic-block.h
>> > +++ b/gcc/basic-block.h
>> > @@ -276,6 +276,57 @@ enum profile_status_d
>> >     fields of this struct are interpreted as the defines for backward
>> >     source compatibility following the definition of this struct.  */
>> >  struct GTY(()) control_flow_graph {
>> > +public:
>> > +  basic_block get_basic_block_by_idx (int idx) const
>> > +  {
>> > +    return (*x_basic_block_info)[idx];
>> > +  }
>>
>> get_basic_block_by_idx is rather long, any reason to not shorten
>> it to get_block () or get_bb?
>
> Will do.   get_bb () then
>
>> > +  void set_basic_block_by_idx (int idx, basic_block bb)
>>
>> set_block () or set_bb()
>
> Will do.
>
>> > +  {
>> > +    (*x_basic_block_info)[idx] = bb;
>> > +  }
>> > +
>> > +  basic_block get_entry_block () const { return x_entry_block_ptr; }
>> > +
>> > +  basic_block get_exit_block () const { return x_exit_block_ptr; }
>> > +
>> > +  vec<basic_block, va_gc> *get_basic_block_info () const
>>
>> are they really used outside of the iterators?
>
> The {entry|exit}_block are used in many places, which appears to almost
> all be comparisons of a basic_block to entry/exit.
>
> The get_basic_block_info () is used in just 3 places due to:
>
>   if (basic_block_info->length () < SOME_VALUE)
>     vec_safe_grow_cleared (basic_block_info, SOME_OTHER_VALUE);
>
> so perhaps that's worth abstracting.
> (specifically, in:
>   * gcc/cfgrtl.c: rtl_create_basic_block
>   * gcc/tree-cfg.c: build_gimple_cfg
>   * gcc/tree-cfg.c: create_bb
> )

Yeah.  create_bb should be a CFG hook I suppose and the generic
wrapper doing the vector management.  As a separate patch I suppose.

>
>> > +  {
>> > +    return x_basic_block_info;
>> > +  }
>> > +  vec<basic_block, va_gc> *&get_basic_block_info ()
>> > +  {
>> > +    return x_basic_block_info;
>> > +  }
>> > +
>> > +  int get_n_basic_blocks () const { return x_n_basic_blocks; }
>> > +  int& get_n_basic_blocks () { return x_n_basic_blocks; }
>> > +
>> > +  int get_n_edges () const { return x_n_edges; }
>> > +  int& get_n_edges () { return x_n_edges; }
>> > +
>> > +  int get_last_basic_block () const { return x_last_basic_block; }
>> > +  int& get_last_basic_block () { return x_last_basic_block; }
>>
>> As you can't set_ those I'd drop the get_.  Any reason to not simply
>> drop the x_ in the members and allow direct accesses?  I fear
>> an -O0 compiler will get even more un-debuggable that it is now
>> with all the accessors :/ (undebuggable as in stepping through source)
>
> My thinking here was to make the x_ things be private, to make it easier
> to grep (or breakpoint) where changes happen.
>
> But it sounds like you'd prefer to simply drop the x_ prefixes and have
> direct access, so sure.

Yes, it's a lot easier for debugging that way.

>> > +  vec<basic_block, va_gc> *get_label_to_block_map () const
>> > +  {
>> > +    return x_label_to_block_map;
>> > +  }
>> > +  vec<basic_block, va_gc> *&get_label_to_block_map ()
>> > +  {
>> > +    return x_label_to_block_map;
>> > +  }
>> > +
>> > +  enum profile_status_d get_profile_status () const
>> > +  {
>> > +    return x_profile_status;
>> > +  }
>> > +  void set_profile_status (enum profile_status_d status)
>> > +  {
>> > +    x_profile_status = status;
>> > +  }
>> > +
>> > +public:
>> >    /* Block pointers for the exit and entry of a function.
>> >       These are always the head and tail of the basic block list.  */
>> >    basic_block x_entry_block_ptr;
>> > @@ -328,32 +379,20 @@ struct GTY(()) control_flow_graph {
>> >  #define SET_BASIC_BLOCK_FOR_FUNCTION(FN,N,BB) \
>> >    ((*basic_block_info_for_function(FN))[(N)] = (BB))
>> >
>> > -/* Defines for textual backward source compatibility.  */
>> > -#define ENTRY_BLOCK_PTR                (cfun->cfg->x_entry_block_ptr)
>> > -#define EXIT_BLOCK_PTR         (cfun->cfg->x_exit_block_ptr)
>> > -#define basic_block_info       (cfun->cfg->x_basic_block_info)
>> > -#define n_basic_blocks         (cfun->cfg->x_n_basic_blocks)
>> > -#define n_edges                        (cfun->cfg->x_n_edges)
>> > -#define last_basic_block       (cfun->cfg->x_last_basic_block)
>> > -#define label_to_block_map     (cfun->cfg->x_label_to_block_map)
>> > -#define profile_status         (cfun->cfg->x_profile_status)
>> > -
>> > -#define BASIC_BLOCK(N)         ((*basic_block_info)[(N)])
>> > -#define SET_BASIC_BLOCK(N,BB)  ((*basic_block_info)[(N)] = (BB))
>> > -
>>
>> Replacements with the macros that already exist and expose cfun would
>> have been an obvious patch btw, without trying to refactor things to look
>> like C++ (ugh).
>
> I'd like to make the cfun->cfg dereference explicit if possible, since
> we may want to optimize the repeated derefs away - we know (but the
> compiler doesn't) that cfun->cfg doesn't change.

Ok, I'm finr with that for now.

> So I'll drop the excessive C++-isms, and try another version of this,
> which gets rid of the x_ prefixes, and where the CFG is passed in as the
> macro param, if that sounds reasonable.  (it sounded like the get_bb ()
> method was acceptable)

Yes, get_bb is fine (all macros with arguments should be converted to
inline functions).  Just plain field references should not be wrapped like that
(one advantage with the macro approach was that we could enforce the
use of SET_ macros via making the FOO macros result an rvalue by
wrapping it inside a cast - we do lose that with plain accesses, so whenever
there is already a SET_FOO and FOO variant the conversion should
use inline functions).

Thanks,
Richard.

> Thanks for your comments
> Dave
>

On Wed, 2013-05-29 at 10:06 +0200, Richard Biener wrote:
> On Tue, May 28, 2013 at 8:04 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > On Mon, 2013-05-27 at 15:38 +0200, Richard Biener wrote:
> >> On Sat, May 25, 2013 at 3:02 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> >> >         Eliminate all direct references to "cfun" from macros in
> >> >         basic-block.h and introduce access methods to control_flow_graph
> >> >
> >> >         * basic-block.h (control_flow_graph::get_basic_block_by_idx): New
> >> >         accessor methods.
> >> >         (control_flow_graph::get_entry_block): New method.
> >> >         (control_flow_graph::get_exit_block): New method.
> >> >         (control_flow_graph::get_basic_block_info): New methods.
> >> >         (control_flow_graph::get_n_basic_blocks): New methods.
> >> >         (control_flow_graph::get_n_edges): New methods.
> >> >         (control_flow_graph::get_last_basic_block): New methods.
> >> >         (control_flow_graph::get_label_to_block_map): New methods.
> >> >         (control_flow_graph::get_profile_status): New method.
> >> >         (control_flow_graph::set_profile_status): New method.
> >> >         (ENTRY_BLOCK_PTR): Eliminate this macro.
> >> >         (EXIT_BLOCK_PTR): Likewise.
> >> >         (basic_block_info): Likewise.
> >> >         (n_basic_blocks): Likewise.
> >> >         (n_edges): Likewise.
> >> >         (last_basic_block): Likewise.
> >> >         (label_to_block_map): Likewise.
> >> >         (profile_status): Likewise.
> >> >         (BASIC_BLOCK): Likewise.
> >> >         (SET_BASIC_BLOCK): Likewise.
> >> >         (FOR_EACH_BB_FN): Rewrite in terms of...
> >> >         (FOR_EACH_BB_CFG): New macro
> >> >         (FOR_EACH_BB): Eliminate this macro
> >> >         (FOR_EACH_BB_REVERSE_FN): Rewrite in terms of...
> >> >         (FOR_EACH_BB_REVERSE_FN_CFG): New macro
> >> >         (FOR_EACH_BB_REVERSE): Eliminate this macro
> >> >         (FOR_ALL_BB): Likewise.
> >> >         (FOR_ALL_BB_CFG): New macro
> >>
> >> I don't like the mix of _CFG / _FN.  It's obvious we are talking about
> >> the CFG of a function in 'FOR_EACH_BB' and friends.
> >
> > The current status quo is a pair of macros:
> >
> >   #define FOO_FN(FN) ...do something on (FN)->cfg
> >   #define FOO() FOO_FN(cfun)
> >
> > My patch changed these to be:
> >
> >   #define FOO_CFG(CFG) ...do something on CFG
> >   #define FOO_FN(FN) FOO_CFG((FN)->cfg)
> >
> > and to get rid of the FOO() with its cfun usage.
> >
> > So would your preference be for the FOO() to mean the cfg:
> >
> >   #define FOO(CFG) ...do something on cfg
> >   #define FOO_FN(FN) FOO((FN)->cfg)
> >
> > ?
> >
> > e.g.
> >
> > #define FOR_EACH_BB(BB, CFG) \
> >   FOR_BB_BETWEEN (BB, (CFG)->x_entry_block_ptr->next_bb, (CFG)->x_exit_block_ptr, next_bb)
> >
> > #define FOR_EACH_BB_FN(BB, FN) FOR_EACH_BB_CFG (BB, (FN)->cfg)
> 
> I suppose the CFG vs. FN distinction is because there is a
> control-flow-graph sub-structure in a function (OTOH there is at most
> a single CFG in a FN, so specifying FN is unambiguous).
> 
> Yes, the un-suffixed variant (FOO) should be the one with the "natural"
> interface (taking a CFG).  People already know about the _FN variant
> and if you change all existing uses of FOO (without cfg/fn argument)
> then that would be the best change.
> 
> Ideally FN or CFG would be handled transparently via overloading
> for example (or giving both struct function and struct cfg the same
> set of member functions), so
> 
> inline basic_block entry_block_ptr (struct function *fn)
> {
>   return fn->cfg->x_entry_block_ptr;
> }
> inline basic_block entry_block_ptr (struct cfg *)
> {
>   return cfg->x_entry_block_ptr;
> }
> 
> #define FOR_EACH_BB (BB, CONTEXT) FOR_BB_BETWEEN (BB, entry_block_ptr
> (CONTEXT)->next_bb, exit_block_ptr (CONTEXT), next_bb)
> 
> and only retain a single macro/function form for context kinds that can
> be unambiguously interchanged.
> 
> Or do people think that would be too confusing?  (I suppose defining an
> automatic conversion from struct function * to struct cfg * is also techincally
> possible, but that only works with the non-member-function way and would
> avoid the overloading).
> 
> That said, I have no strong preference to remove the FOO_FN variants together
> with this patch.  Using FOO with cfg arguments and FOO_FN with function
> arguments is fine for me (eventually as intermediate step).
> 
> >> > ---
> >> > diff --git a/gcc/basic-block.h b/gcc/basic-block.h
> >> > index eed320c..3949417 100644
> >> > --- a/gcc/basic-block.h
> >> > +++ b/gcc/basic-block.h
> >> > @@ -276,6 +276,57 @@ enum profile_status_d
> >> >     fields of this struct are interpreted as the defines for backward
> >> >     source compatibility following the definition of this struct.  */
> >> >  struct GTY(()) control_flow_graph {
> >> > +public:
> >> > +  basic_block get_basic_block_by_idx (int idx) const
> >> > +  {
> >> > +    return (*x_basic_block_info)[idx];
> >> > +  }
> >>
> >> get_basic_block_by_idx is rather long, any reason to not shorten
> >> it to get_block () or get_bb?
> >
> > Will do.   get_bb () then
> >
> >> > +  void set_basic_block_by_idx (int idx, basic_block bb)
> >>
> >> set_block () or set_bb()
> >
> > Will do.
> >
> >> > +  {
> >> > +    (*x_basic_block_info)[idx] = bb;
> >> > +  }
> >> > +
> >> > +  basic_block get_entry_block () const { return x_entry_block_ptr; }
> >> > +
> >> > +  basic_block get_exit_block () const { return x_exit_block_ptr; }
> >> > +
> >> > +  vec<basic_block, va_gc> *get_basic_block_info () const
> >>
> >> are they really used outside of the iterators?
> >
> > The {entry|exit}_block are used in many places, which appears to almost
> > all be comparisons of a basic_block to entry/exit.
> >
> > The get_basic_block_info () is used in just 3 places due to:
> >
> >   if (basic_block_info->length () < SOME_VALUE)
> >     vec_safe_grow_cleared (basic_block_info, SOME_OTHER_VALUE);
> >
> > so perhaps that's worth abstracting.
> > (specifically, in:
> >   * gcc/cfgrtl.c: rtl_create_basic_block
> >   * gcc/tree-cfg.c: build_gimple_cfg
> >   * gcc/tree-cfg.c: create_bb
> > )
> 
> Yeah.  create_bb should be a CFG hook I suppose and the generic
> wrapper doing the vector management.  As a separate patch I suppose.
> 
> >
> >> > +  {
> >> > +    return x_basic_block_info;
> >> > +  }
> >> > +  vec<basic_block, va_gc> *&get_basic_block_info ()
> >> > +  {
> >> > +    return x_basic_block_info;
> >> > +  }
> >> > +
> >> > +  int get_n_basic_blocks () const { return x_n_basic_blocks; }
> >> > +  int& get_n_basic_blocks () { return x_n_basic_blocks; }
> >> > +
> >> > +  int get_n_edges () const { return x_n_edges; }
> >> > +  int& get_n_edges () { return x_n_edges; }
> >> > +
> >> > +  int get_last_basic_block () const { return x_last_basic_block; }
> >> > +  int& get_last_basic_block () { return x_last_basic_block; }
> >>
> >> As you can't set_ those I'd drop the get_.  Any reason to not simply
> >> drop the x_ in the members and allow direct accesses?  I fear
> >> an -O0 compiler will get even more un-debuggable that it is now
> >> with all the accessors :/ (undebuggable as in stepping through source)
> >
> > My thinking here was to make the x_ things be private, to make it easier
> > to grep (or breakpoint) where changes happen.
> >
> > But it sounds like you'd prefer to simply drop the x_ prefixes and have
> > direct access, so sure.
> 
> Yes, it's a lot easier for debugging that way.
> 
> >> > +  vec<basic_block, va_gc> *get_label_to_block_map () const
> >> > +  {
> >> > +    return x_label_to_block_map;
> >> > +  }
> >> > +  vec<basic_block, va_gc> *&get_label_to_block_map ()
> >> > +  {
> >> > +    return x_label_to_block_map;
> >> > +  }
> >> > +
> >> > +  enum profile_status_d get_profile_status () const
> >> > +  {
> >> > +    return x_profile_status;
> >> > +  }
> >> > +  void set_profile_status (enum profile_status_d status)
> >> > +  {
> >> > +    x_profile_status = status;
> >> > +  }
> >> > +
> >> > +public:
> >> >    /* Block pointers for the exit and entry of a function.
> >> >       These are always the head and tail of the basic block list.  */
> >> >    basic_block x_entry_block_ptr;
> >> > @@ -328,32 +379,20 @@ struct GTY(()) control_flow_graph {
> >> >  #define SET_BASIC_BLOCK_FOR_FUNCTION(FN,N,BB) \
> >> >    ((*basic_block_info_for_function(FN))[(N)] = (BB))
> >> >
> >> > -/* Defines for textual backward source compatibility.  */
> >> > -#define ENTRY_BLOCK_PTR                (cfun->cfg->x_entry_block_ptr)
> >> > -#define EXIT_BLOCK_PTR         (cfun->cfg->x_exit_block_ptr)
> >> > -#define basic_block_info       (cfun->cfg->x_basic_block_info)
> >> > -#define n_basic_blocks         (cfun->cfg->x_n_basic_blocks)
> >> > -#define n_edges                        (cfun->cfg->x_n_edges)
> >> > -#define last_basic_block       (cfun->cfg->x_last_basic_block)
> >> > -#define label_to_block_map     (cfun->cfg->x_label_to_block_map)
> >> > -#define profile_status         (cfun->cfg->x_profile_status)
> >> > -
> >> > -#define BASIC_BLOCK(N)         ((*basic_block_info)[(N)])
> >> > -#define SET_BASIC_BLOCK(N,BB)  ((*basic_block_info)[(N)] = (BB))
> >> > -
> >>
> >> Replacements with the macros that already exist and expose cfun would
> >> have been an obvious patch btw, without trying to refactor things to look
> >> like C++ (ugh).
> >
> > I'd like to make the cfun->cfg dereference explicit if possible, since
> > we may want to optimize the repeated derefs away - we know (but the
> > compiler doesn't) that cfun->cfg doesn't change.
> 
> Ok, I'm finr with that for now.
> 
> > So I'll drop the excessive C++-isms, and try another version of this,
> > which gets rid of the x_ prefixes, and where the CFG is passed in as the
> > macro param, if that sounds reasonable.  (it sounded like the get_bb ()
> > method was acceptable)
> 
> Yes, get_bb is fine (all macros with arguments should be converted to
> inline functions).  Just plain field references should not be wrapped like that
> (one advantage with the macro approach was that we could enforce the
> use of SET_ macros via making the FOO macros result an rvalue by
> wrapping it inside a cast - we do lose that with plain accesses, so whenever
> there is already a SET_FOO and FOO variant the conversion should
> use inline functions).

Thanks.  Jeff suggested in another mail that I start with just a smaller
patch, to just remove one of the macro, so I've posted that as:
  http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01878.html

(just removing the n_basic_blocks macro).

If that's accepted, I can send further patches to that remove each of
the various cfun macros, based on the above feedback, and then patches
to consolidate some of the "cfun->cfg->" lookups where it doesn't change
inside a function.

Dave

I like the idea that someone reviews the end destination you want to get to, and the general path you want to use to get there, and then just pre-approve the entire concept.  :-)  I think I like the end destination; the path you have chosen is slightly different from the one I might have chosen, but the path you've pick has the nice property that you can incrementally get to the end and keep the tree working; so, likely is superior to the path I was thinking about.  The path I was thinking about, was to start by forming classes, and hoist the data up into the classes, and get a this parameter passed around, then something like n_basic_blocks in the code, becomes n_basic_blocks in the code, with a implicit reference to the this parameter, or, to cfg->n_basic_blocks, should the type of this not be cfg.  Cause the transformations are just identity, the disruption to the code is less (in some ways).  However there is a larger disruption in that all the functions quickly become member functions, though, that also lies closer to the end result I had in mind as well.