From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82715 invoked by alias); 29 Oct 2015 09:49:35 -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 82705 invoked by uid 89); 29 Oct 2015 09:49:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Thu, 29 Oct 2015 09:49:32 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 13511AC09 for ; Thu, 29 Oct 2015 09:50:00 +0000 (UTC) Subject: Re: [PATCH] Pass manager: add support for termination of pass list To: gcc-patches@gcc.gnu.org References: <56263B07.1010900@suse.cz> <562758A9.3070309@suse.cz> <562775F3.1050609@suse.cz> <5628C262.8000205@suse.cz> <562F6FC6.1020200@suse.cz> <562F9889.2070704@suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <5631EBA9.2040105@suse.cz> Date: Thu, 29 Oct 2015 10:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg03138.txt.bz2 On 10/28/2015 04:23 PM, Richard Biener wrote: > On Tue, Oct 27, 2015 at 4:30 PM, Martin Liška wrote: >> On 10/27/2015 03:49 PM, Richard Biener wrote: >>> On Tue, Oct 27, 2015 at 1:36 PM, Martin Liška wrote: >>>> On 10/26/2015 02:48 PM, Richard Biener wrote: >>>>> On Thu, Oct 22, 2015 at 1:02 PM, Martin Liška wrote: >>>>>> On 10/21/2015 04:06 PM, Richard Biener wrote: >>>>>>> On Wed, Oct 21, 2015 at 1:24 PM, Martin Liška wrote: >>>>>>>> On 10/21/2015 11:59 AM, Richard Biener wrote: >>>>>>>>> On Wed, Oct 21, 2015 at 11:19 AM, Martin Liška wrote: >>>>>>>>>> On 10/20/2015 03:39 PM, Richard Biener wrote: >>>>>>>>>>> On Tue, Oct 20, 2015 at 3:00 PM, Martin Liška wrote: >>>>>>>>>>>> Hello. >>>>>>>>>>>> >>>>>>>>>>>> As part of upcoming merge of HSA branch, we would like to have possibility to terminate >>>>>>>>>>>> pass manager after execution of the HSA generation pass. The HSA back-end is implemented >>>>>>>>>>>> as a tree pass that directly emits HSAIL from gimple tree representation. The pass operates >>>>>>>>>>>> on clones created by HSA IPA pass and the pass manager should stop execution of further >>>>>>>>>>>> RTL passes. >>>>>>>>>>>> >>>>>>>>>>>> Suggested patch survives bootstrap and regression tests on x86_64-linux-pc. >>>>>>>>>>>> >>>>>>>>>>>> What do you think about it? >>>>>>>>>>> >>>>>>>>>>> Are you sure it works this way? >>>>>>>>>>> >>>>>>>>>>> Btw, you will miss executing of all the cleanup passes that will >>>>>>>>>>> eventually free memory >>>>>>>>>>> associated with the function. So I'd rather support a >>>>>>>>>>> TODO_discard_function which >>>>>>>>>>> should basically release the body from the cgraph. >>>>>>>>>> >>>>>>>>>> Hi. >>>>>>>>>> >>>>>>>>>> Agree with you that I should execute all TODOs, which can be easily done. >>>>>>>>>> However, if I just try to introduce the suggested TODO and handle it properly >>>>>>>>>> by calling cgraph_node::release_body, then for instance fn->gimple_df, fn->cfg are >>>>>>>>>> released and I hit ICEs on many places. >>>>>>>>>> >>>>>>>>>> Stopping the pass manager looks necessary, or do I miss something? >>>>>>>>> >>>>>>>>> "Stopping the pass manager" is necessary after TODO_discard_function, yes. >>>>>>>>> But that may be simply done via a has_body () check then? >>>>>>>> >>>>>>>> Thanks, there's second version of the patch. I'm going to start regression tests. >>>>>>> >>>>>>> As release_body () will free cfun you should pop_cfun () before >>>>>>> calling it (and thus >>>>>> >>>>>> Well, release_function_body calls both push & pop, so I think calling pop >>>>>> before cgraph_node::release_body is not necessary. >>>>> >>>>> (ugh). >>>>> >>>>>> If tried to call pop_cfun before cgraph_node::release_body, I have cfun still >>>>>> pointing to the same (function *) (which is gcc_freed, but cfun != NULL). >>>>> >>>>> Hmm, I meant to call pop_cfun then after it (unless you want to fix the above, >>>>> none of the freeing functions should techincally need 'cfun', just add >>>>> 'fn' parameters ...). >>>>> >>>>> I expected pop_cfun to eventually set cfun to NULL if it popped the >>>>> "last" cfun. Why >>>>> doesn't it do that? >>>>> >>>>>>> drop its modification). Also TODO_discard_functiuon should be only set for >>>>>>> local passes thus you should probably add a gcc_assert (cfun). >>>>>>> I'd move its handling earlier, definitely before the ggc_collect, eventually >>>>>>> before the pass_fini_dump_file () (do we want a last dump of the >>>>>>> function or not?). >>>>>> >>>>>> Fully agree, moved here. >>>>>> >>>>>>> >>>>>>> @@ -2397,6 +2410,10 @@ execute_pass_list_1 (opt_pass *pass) >>>>>>> { >>>>>>> gcc_assert (pass->type == GIMPLE_PASS >>>>>>> || pass->type == RTL_PASS); >>>>>>> + >>>>>>> + >>>>>>> + if (!gimple_has_body_p (current_function_decl)) >>>>>>> + return; >>>>>>> >>>>>>> too much vertical space. With popping cfun before releasing the body the check >>>>>>> might just become if (!cfun) and >>>>>> >>>>>> As mentioned above, as release body is symmetric (calling push & pop), the suggested >>>>>> guard will not work. >>>>> >>>>> I suggest to fix it. If it calls push/pop it should leave with the >>>>> original cfun, popping again >>>>> should make it NULL? >>>>> >>>>>>> >>>>>>> @@ -2409,7 +2426,7 @@ execute_pass_list (function *fn, opt_pass *pass) >>>>>>> { >>>>>>> push_cfun (fn); >>>>>>> execute_pass_list_1 (pass); >>>>>>> - if (fn->cfg) >>>>>>> + if (gimple_has_body_p (current_function_decl) && fn->cfg) >>>>>>> { >>>>>>> free_dominance_info (CDI_DOMINATORS); >>>>>>> free_dominance_info (CDI_POST_DOMINATORS); >>>>>>> >>>>>>> here you'd need to guard the pop_cfun call on cfun != NULL. IMHO it's better >>>>>>> to not let cfun point to deallocated data. >>>>>> >>>>>> As I've read the code, since we gcc_free 'struct function', one can just rely on >>>>>> gimple_has_body_p (current_function_decl) as it correctly realizes that >>>>>> DECL_STRUCT_FUNCTION (current_function_decl) == NULL. >>>>> >>>>> I'm sure that with some GC checking ggc_free makes them #deadbeef or so: >>>>> >>>>> void >>>>> ggc_free (void *p) >>>>> { >>>>> ... >>>>> #ifdef ENABLE_GC_CHECKING >>>>> /* Poison the data, to indicate the data is garbage. */ >>>>> VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (p, size)); >>>>> memset (p, 0xa5, size); >>>>> #endif >>>>> >>>>> so I don't think that's a good thing to rely on ;) >>>>> >>>>> Richard. >>>> >>>> Hi Richi, fully agree that relying of 0xdeadbeaf is not good. >>>> I'm sending quite simple patch v4 where I enable popping up >>>> the stack to eventually set cfun = current_function_decl = NULL. >>>> >>>> Question of using push & pop in cgraph_node::release_body should >>>> be orthogonal as there are other places where the function is used. >>>> >>>> What do you think about the patch? >>> >>> @@ -4763,6 +4763,13 @@ push_cfun (struct function *new_cfun) >>> void >>> pop_cfun (void) >>> { >>> + if (cfun_stack.is_empty ()) >>> + { >>> + set_cfun (NULL); >>> + current_function_decl = NULL_TREE; >>> + return; >>> + } >>> + >>> >>> I'd like to avoid this by... >>> >>> @@ -2405,11 +2428,12 @@ execute_pass_list (function *fn, opt_pass *pass) >>> { >>> push_cfun (fn); >>> execute_pass_list_1 (pass); >>> - if (fn->cfg) >>> + if (current_function_decl && fn->cfg) >>> { >>> free_dominance_info (CDI_DOMINATORS); >>> free_dominance_info (CDI_POST_DOMINATORS); >>> } >>> + >>> pop_cfun (); >>> >>> making this conditional on if (cfun). Btw, please check cfun against NULL, >>> not current_function_decl. >> >> Done. >> >>> >>> + if (dom_info_available_p (CDI_POST_DOMINATORS)) >>> + free_dominance_info (CDI_POST_DOMINATORS); >>> + >>> + /* Pop function inserted in execute_pass_list. */ >>> + pop_cfun (); >>> + >>> + cgraph_node::get (cfun->decl)->release_body (); >>> + >>> + /* Set cfun and current_function_decl to be NULL. */ >>> + pop_cfun (); >>> + } >>> >>> this also looks weird. Because you pop_cfun and then access cfun and >>> then you pop cfun again? I'd say most clean would be >>> >>> + if (dom_info_available_p (CDI_POST_DOMINATORS)) >>> + free_dominance_info (CDI_POST_DOMINATORS); >>> + tree fn = cfun->decl; >>> pop_cfun (); >>> cgraph_node::get (fn)->release_body (); >>> } >>> >>> or does the comment say that the current function is on the stack >>> twice somehow? How can that happen? >> >> You are right, your change applied. >> >> Sending V5. If OK, I'll create corresponding changelog entry >> and run regression tests. > > You still have > > @@ -4763,6 +4763,13 @@ push_cfun (struct function *new_cfun) > void > pop_cfun (void) > { > + if (cfun_stack.is_empty ()) > + { > + set_cfun (NULL); > + current_function_decl = NULL_TREE; > + return; > + } > > and popping of cfun twice in execute_one_pass. > > Richard. Right and that's the way how I set current_function_decl = cfun = NULL (both pop_cfun are commented what they do). As the popping is done at the end of execute_pass_list and having empty stack just sets to NULL, it should be fine. Martin > >> Thanks, >> Martin >> >>> >>> Richard. >>> >>> >>>> Thanks, >>>> Martin >>>> >>>>> >>>>>> I'm attaching v3. >>>>>> >>>>>> Thanks, >>>>>> Martin >>>>>> >>>>>>> >>>>>>> Otherwise looks good to me. >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>> >>>>>>>> Martin >>>>>>>> >>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Martin >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Richard. >>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Martin >>>>>>>>>> >>>>>>>> >>>>>> >>>> >>