On Tue, May 28, 2013 at 8:04 PM, David Malcolm <dmalc...@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 <dmalc...@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
>

Reply via email to