From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93833 invoked by alias); 14 Aug 2015 18:20:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 93821 invoked by uid 89); 14 Aug 2015 18:20:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: fencepost.gnu.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (208.118.235.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 14 Aug 2015 18:20:26 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49741) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1ZQJaZ-0000gp-Qn for gcc-patches@gnu.org; Fri, 14 Aug 2015 14:20:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZQJaW-0001Vv-6S for gcc-patches@gnu.org; Fri, 14 Aug 2015 14:20:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50268) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQJaV-0001Vb-U4 for gcc-patches@gnu.org; Fri, 14 Aug 2015 14:20:20 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 4407796D2; Fri, 14 Aug 2015 18:20:19 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-104.phx2.redhat.com [10.3.113.104]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t7EIKI72029725; Fri, 14 Aug 2015 14:20:18 -0400 Subject: Re: [PATCH 1/2] C++-ify dominance.c To: Mikhail Maltsev , gcc-patches References: <55CD3C87.40101@gmail.com> From: Jeff Law Message-ID: <55CE3162.5030400@redhat.com> Date: Fri, 14 Aug 2015 18:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <55CD3C87.40101@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00821.txt.bz2 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 > > * (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