From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71249 invoked by alias); 15 Aug 2015 06:05:38 -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 71236 invoked by uid 89); 15 Aug 2015 06:05:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KAM_ASCII_DIVIDERS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=no 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; Sat, 15 Aug 2015 06:05:35 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52543) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1ZQUay-0000zR-RA for gcc-patches@gnu.org; Sat, 15 Aug 2015 02:05:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZQUat-0004ov-DU for gcc-patches@gnu.org; Sat, 15 Aug 2015 02:05:32 -0400 Received: from mail-la0-x22c.google.com ([2a00:1450:4010:c03::22c]:34490) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZQUas-0004ol-VX for gcc-patches@gnu.org; Sat, 15 Aug 2015 02:05:27 -0400 Received: by labd1 with SMTP id d1so54550177lab.1 for ; Fri, 14 Aug 2015 23:05:26 -0700 (PDT) X-Received: by 10.112.65.35 with SMTP id u3mr46950407lbs.103.1439618726139; Fri, 14 Aug 2015 23:05:26 -0700 (PDT) Received: from [192.168.123.200] ([77.41.78.126]) by smtp.googlemail.com with ESMTPSA id ad1sm2006618lbc.5.2015.08.14.23.05.25 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 14 Aug 2015 23:05:25 -0700 (PDT) Message-ID: <55CED6A4.3080909@gmail.com> Date: Sat, 15 Aug 2015 06:13:00 -0000 From: Mikhail Maltsev User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Richard Biener CC: gcc-patches , Jeff Law , David Malcolm Subject: Re: [PATCH 2/2] Get rid of global state accesses in dominance.c References: <55CD3C87.40101@gmail.com> <55CD3EA1.1050104@gmail.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------070004050901000601050300" X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c03::22c X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00841.txt.bz2 This is a multi-part message in MIME format. --------------070004050901000601050300 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 3544 On 08/14/2015 11:02 AM, Richard Biener wrote: > So the last time I did similar refactoring I wondered if we can somehow avoid > the "noise" in non-IPA passes. Ideas I came up with are > > a) Inherit gimple/rtl pass classes from a class which is initialized with the > function the pass operates on and provides methods like > > bool dom_info_available_p (..) { return dom_info_available_p (fn, ...); } > > thus wraps APIs working on a specific function. > > b) Do sth similar but make it work with overloads and clever (no idea what!) > C++ that disables them if this_fn cannot be looked up > > template > bool dom_info_available_p (..., struct function *fn = this_fn); > I attached some sketch of what this "clever C++" could look like. It still requires some changes in classes which would use this new base class, but they are not very significant compared to converting pass functions into members of these classes. > all of the above would of course require that passes make all their > implementation > be methods of their pass class. So even more refactoring. The good thing is that this can be done incrementally, i.e. changing one pass at a time. > Refactoring APIs that are used from IPA and make push/pop_cfun necessary > for them are another thing. (there are still plenty left I think) What I could find is: 1. ipa_analyze_node 2. some transactional memory-related stuff: ipa_tm_scan_irr_function, ipa_tm_transform_transaction, ipa_tm_transform_clone, ipa_tm_execute. 3. tree_profiling Presumably there are no more calculate_dominance_info/free_dominance_info call sites with push_cfun/pop_cfun nearby (except for passes.c, but they are not related to IPA). So now I need to figure out, what other global state (which is set up by push_cfun) can these functions use? Could you give some advice about that, in sense, what should I look for (e.g., push_cfun calls some target hooks, but I did not try to look in detail, what kind of actions are performed by it)? As for the API. I propose to, at least, remove uses of cfun from dominance.c itself, but provide helper functions which will allow to leave the callers unchanged, but make it possible to use all dominance-related API on functions other than cfun. Does such change sound reasonable to you? On 08/14/2015 11:11 PM, David Malcolm wrote: > The JIT guards all access to GCC's state in a big mutex ("jit_mutex", in > gcc/jit/jit-playback.c). Yeah, I know, I looked through JIT documentation briefly. > > For example, this includes everything to do with GC and GTY, since > there's implicitly a single set of GC roots and the GC code isn't thread > safe. And GGC is a "stop-the-world" collector, which is just not designed for multithreaded environment. (sigh) As for the patch, I think reviewing this variant does not make much sense, because of Richard suggestion on how to avoid changing the callers. But still thanks for looking. > > I've been tackling things on an as-needed basis - for example, the > recent timevar global-state removal was motivated by wanting to expose a > profiling API to jit client code. > By the way, speaking of timevars. I wrote about my concerns w.r.t. them, but did not get any feedback: https://gcc.gnu.org/ml/gcc/2015-07/msg00165.html Briefly speaking, I noticed that our timers introduce rather significant measurement errors and proposed some ideas about reducing them. What do you think? -- Regards, Mikhail Maltsev --------------070004050901000601050300 Content-Type: text/x-c++src; name="dom_demo.cc" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="dom_demo.cc" Content-length: 4073 #include //===- various headers - Existing GCC code --------------------------------===// // // No changes required //===----------------------------------------------------------------------===// enum cdi_direction { CDI_DOMINATORS, CDI_POSTDOMINATORS }; struct basic_block_def { }; typedef basic_block_def *basic_block; struct function { }; function *cfun; class gimple_opt_pass { public: virtual unsigned int execute(function *) = 0; }; //===- dominance.h - Refactored version -----------------------------------===// // // Allow passing function as an additional parameter (with default value), so // that dominaince.c does not directly use cfun any more. //===----------------------------------------------------------------------===// // We will need cfun to be in scope. Alternatively, we can put a 3-argument // overload of domainted_by_p in another header, where cfun is available. extern function *cfun; bool dominated_by_p(cdi_direction, basic_block, basic_block, function *fun = cfun) { (void)fun; printf("\t4-argument overload\n"); return false; } //===- old_pass.c - This pass uses cfun -----------------------------------===// // // We don't need to change anything here //===----------------------------------------------------------------------===// class old_pass : public gimple_opt_pass { public: virtual unsigned int execute(function *); }; void old_pass_do_stuff() { basic_block bb1 = 0, bb2 = 0; // ... if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) { // ... } } unsigned int old_pass::execute(function *fun) { printf("running old_pass::execute\n"); cfun = fun; old_pass_do_stuff(); printf("finished old_pass::execute\n"); return 0; } //===- dom_info_mixin.h - Header file for the new API ---------------------===// // // dom_info_mixin template provides helper methods for manipulating // dominance information of the function currently being compiled //===----------------------------------------------------------------------===// template class dom_info_mixin { protected: bool dominated_by_p(cdi_direction dir, basic_block bb1, basic_block bb2) const { printf("\tdom_info_mixin: using m_this_fn ... -> "); return ::dominated_by_p(dir, bb1, bb2, static_cast(this)->m_this_fn); } }; // protection against unintended use of cfun (see new_pass_do_other_stuff) bool dominated_by_p(cdi_direction dir, basic_block bb1, basic_block bb2); //===- new_pass.cc - Example pass which uses the new API ------------------===// // // new_pass stores a pointer to current function in m_this_fn (so we get rid // of the global shared state - at least some part of it) and uses small // accessor methods, so we don't have to write "m_this_fn" every time. //===----------------------------------------------------------------------===// class new_pass : public gimple_opt_pass, public dom_info_mixin { public: friend class dom_info_mixin; // or just make m_this_fn public virtual unsigned int execute(function *); private: void do_stuff(); function *m_this_fn; }; static void new_pass_do_other_stuff(); unsigned int new_pass::execute(function *fun) { printf("running new_pass::execute\n"); m_this_fn = fun; do_stuff(); new_pass_do_other_stuff(); printf("finished new_pass::execute\n"); return 0; } void new_pass::do_stuff() { basic_block bb1 = 0, bb2 = 0; // ... if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) { // ... } } static void new_pass_do_other_stuff() { #if 0 basic_block bb1 = 0, bb2 = 0; // ... // error: call of overloaded ‘dominated_by_p(cdi_direction, basic_block_def*&, basic_block_def*&)’ is ambiguous if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) { // ... } #endif } int main() { function fun; old_pass old_pass_instance; old_pass_instance.execute(&fun); new_pass new_pass_instance; new_pass_instance.execute(&fun); return 0; } --------------070004050901000601050300--