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

[Richard Biener <richard dot guenther at gmail dot com>]

On August 14, 2015 8:20:18 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>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?

Na, I was thinking of passes that need to update dominance info repeatedly for subgraphs.  Those could use this facility and then indeed make sure to drop the whole function one after they are done.

Richard.

>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