From patchwork Tue Oct 27 12:36:22 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 536602 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 1DC88141308 for ; Tue, 27 Oct 2015 23:36:44 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=kqfwjP9X; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=ErJ3J/gXOeTgYWGqJ xPmLNvbGAoNnW4VIqlsNVWoHydGrApwHgv6MJu8SH7MGJuD/CaXjjksyn2IAPttp ixWwBHMBmQwAJ4IRGIQHTRTKtaZ0RaONmSjx6OlCgwfmUxe6rX8lXWSVKrUW6Zak Gdo34CFufZ/6q6/QBc6Q8QncqY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=Zh0OZq+5nIwDCVeWetQDKi9 rJmg=; b=kqfwjP9XYsShn3xKEuqZb9nePXPmYk4CxXwVi1QATdx0loleL4oDzgk /SU1J5hmBVpmUT8YnLYvQZtS4RFhE6NcqfWQqP4hxjEs2sQOizSyeALCnNDxsQME GFg9eqYDHEwMoPWK7U0b3uQQSXV3cjXEMXrph8f6bPCJaUHspxOc= Received: (qmail 121392 invoked by alias); 27 Oct 2015 12:36:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 121378 invoked by uid 89); 27 Oct 2015 12:36:36 -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; Tue, 27 Oct 2015 12:36:30 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5E5C4AC6A for ; Tue, 27 Oct 2015 12:36:26 +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> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <562F6FC6.1020200@suse.cz> Date: Tue, 27 Oct 2015 13:36:22 +0100 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: X-IsSubscribed: yes 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? Thanks, Martin > >> I'm attaching v3. >> >> Thanks, >> Martin >> >>> >>> Otherwise looks good to me. >>> >>> Richard. >>> >>> >>>> Martin >>>> >>>>> >>>>>> Thanks, >>>>>> Martin >>>>>> >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>>> Thanks, >>>>>>>> Martin >>>>>> >>>> >> From 4d02a44cef73dbdced230b8b3a80183b9bcca264 Mon Sep 17 00:00:00 2001 From: marxin Date: Thu, 22 Oct 2015 12:46:16 +0200 Subject: [PATCH] Version 4. --- gcc/function.c | 7 +++++++ gcc/hsa-gen.c | 3 +-- gcc/passes.c | 26 +++++++++++++++++++++++++- gcc/tree-pass.h | 3 +++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/gcc/function.c b/gcc/function.c index db5bc1c..6878e9d 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -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; + } + struct function *new_cfun = cfun_stack.pop (); /* When in_dummy_function, we do have a cfun but current_function_decl is NULL. We also allow pushing NULL cfun and subsequently changing diff --git a/gcc/passes.c b/gcc/passes.c index 8b3fb2f..87dd25f 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -2378,6 +2378,26 @@ execute_one_pass (opt_pass *pass) current_pass = NULL; + if (todo_after & TODO_discard_function) + { + gcc_assert (cfun); + /* As cgraph_node::release_body expects release dominators info, + we have to release it. */ + if (dom_info_available_p (CDI_DOMINATORS)) + free_dominance_info (CDI_DOMINATORS); + + 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 (); + } + /* Signal this is a suitable GC collection point. */ if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect)) ggc_collect (); @@ -2392,6 +2412,9 @@ execute_pass_list_1 (opt_pass *pass) { gcc_assert (pass->type == GIMPLE_PASS || pass->type == RTL_PASS); + + if (current_function_decl == NULL) + return; if (execute_one_pass (pass) && pass->sub) execute_pass_list_1 (pass->sub); @@ -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 (); } diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index 7a5f476..2627df3 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -296,6 +296,9 @@ protected: /* Rebuild the callgraph edges. */ #define TODO_rebuild_cgraph_edges (1 << 22) +/* Release function body and stop pass manager. */ +#define TODO_discard_function (1 << 23) + /* Internally used in execute_function_todo(). */ #define TODO_update_ssa_any \ (TODO_update_ssa \ -- 2.6.2