Re: [PATCH 1/2] C++-ify dominance.c

[Jeff Law <law@redhat.com>]

On 08/13/2015 06:55 PM, Mikhail Maltsev wrote:
> Hi all.
>
> These two patches are refactoring of dominator-related code.
>
> The comment in dominance.c says: "We work in a poor-mans object oriented
> fashion, and carry an instance of this structure through all our 'methods'". So,
> the first patch converts the mentioned structure (dom_info) into a class with
> proper encapsulation. It also adds a new member - m_fn (the function currently
> being compiled) to this structure and replaces some uses of cfun with m_fn. It
> also contains some fixes, related to current coding standards: move variable
> declarations to place of first use, replace elaborated type specifiers (i.e.
> "struct/enum foo") by simple ones (i.e., just "foo") in function prototypes.
>
> Bootstrapped and regtested on x86_64-linux. Tested build of config-list.mk.
>
> gcc/ChangeLog:
>
> 2015-08-14  Mikhail Maltsev<maltsevm@gmail.com>
>
>          * (ENABLE_CHECKING): Define as 0 by default.
>          dominance.c (new_zero_array): Define.
>          (dom_info): Define as class instead of struct.
>          (dom_info::dom_info, ~dom_info): Define.  Use new/delete for memory
>          allocations/deallocations.  Pass function as parameter (instead of
>          using cfun).
>          (dom_info::get_idom): Define accessor method.
>          (dom_info::calc_dfs_tree_nonrec, calc_dfs_tree, compress, eval,
>          link_roots, calc_idoms): Redefine as class members.  Use m_fn instead
>          of cfun.
>          (init_dom_info, free_dom_info): Remove (use dom_info ctor/dtor).
>          (dom_convert_dir_to_idx): Fix prototype.
>          (assign_dfs_numbers): Move variable declarations to their first uses.
>          (calculate_dominance_info): Remove conditional compilation, move
>          variables.
>          (free_dominance_info, get_immediate_dominator, set_immediate_dominator,
>          get_dominated_b, get_dominated_by_region, get_dominated_to_depth,
>          redirect_immediate_dominators, nearest_common_dominator_for_set,
>          dominated_by_p, bb_dom_dfs_in, bb_dom_dfs_out, verify_dominators,
>          determine_dominators_for_sons, iterate_fix_dominators, first_dom_son,
>          next_dom_son, debug_dominance_info, debug_dominance_tree_1): Adjust to
>          use class dom_info. Move variable declarations to the place of first
>          use. Fix prototypes (remove struct/enum).
>          * dominance.h: Fix prototypes (remove struct/enum).
>
> -- Regards, Mikhail Maltsev
>
It looks like your patch is primarily concerned with converting all the 
internal stuff into a C++ style and not exposing a class to the users of 
dominance.h. Correct?

As a whole I don't see anything objectionable here, but I also don't see 
that it really takes us forward in a real significant way.  I guess 
there's some value in having dominance.c brought up to current 
standards, but my recollection was we weren't going to do through the 
entire source base and do things like move variable declarations to 
their initial use and more generally c++-ify the code base en-masse.

Similarly losing the elaborated type specifiers doesn't really gain us 
anything, except perhaps one less token when people parse the code. 
Again, not objectionable, but also not a big gain.

I could argue that those kind of changes are independent of turning 
dom_info into a real class and if they're going to go forward, they 
would have to stand alone on their merits and go in independently if 
turning dom_info into a class (which AFIACT is the meat of this patch).



> refactor_dom1.patch
>
>
> diff --git a/gcc/dominance.c b/gcc/dominance.c
> index d8d87ca..3c4f228 100644
> --- a/gcc/dominance.c
> +++ b/gcc/dominance.c
> @@ -44,6 +44,10 @@
>   #include "timevar.h"
>   #include "graphds.h"
>
> +#ifndef ENABLE_CHECKING
> +# define ENABLE_CHECKING 0
> +#endif
Umm, isn't ENABLE_CHECKING defined in auto-host.h (as set up by 
configure?)  What's the reason for this change?

Is the issue that auto-host.h won't define checking at all for 
--disable-checking?

I think that the ENABLE_CHECKING conversion from #ifdef testing to 
testing for a value should probably be done separately.  It also 
probably has higher value than this refactoring.


> +
> +  /* The function being processed.  */
> +  function *m_fn;
So presumably the idea here is to avoid explicitly hitting cfun which in 
theory we could recompute the dominance tree for another function. But 
is that really all that useful?

I'm a bit torn here.  Richi mentioned the idea of stuffing away a 
pointer to cfun looked backwards to him and he'd pretty stuffing away 
the entry, exit & # blocks and perhaps take us a step towards the 
ability to compute dominance on sub-graphs.

The problem I see with Richi's idea now that I think about it more is 
keeping that information up-to-date.  Ie, if we've stuffed away those 
pointers, what happens if (for example) a block gets deleted from the 
graph.  What if that block happens to be the exit block we've recorded?

So I guess I'm starting to lean towards saving away the cfun  like as is 
done in this patch.


>> +  int *son      = new int[n + 1],
> +      *brother  = new int[n + 1],
> +      *parent   = new int[n + 1];
ICK.  Don't do this.  Make each initialization a separate statement. 
There's nothing really to be gained by avoiding the "int" here.

So ultimately the question is whether or not we're gaining much with 
this patch to justify the churn it creates.  I think I'll hold off on 
yes/no to the patch to give other folks an opportunity to chime in.


Jeff