On Wed, 2013-05-29 at 10:06 +0200, Richard Biener wrote:
> 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. 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
