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