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<malts...@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
