| Message ID | 5628C262.8000205@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 22, 2015 at 1:02 PM, Martin Liška <mliska@suse.cz> wrote: > On 10/21/2015 04:06 PM, Richard Biener wrote: >> On Wed, Oct 21, 2015 at 1:24 PM, Martin Liška <mliska@suse.cz> wrote: >>> On 10/21/2015 11:59 AM, Richard Biener wrote: >>>> On Wed, Oct 21, 2015 at 11:19 AM, Martin Liška <mliska@suse.cz> wrote: >>>>> On 10/20/2015 03:39 PM, Richard Biener wrote: >>>>>> On Tue, Oct 20, 2015 at 3:00 PM, Martin Liška <mliska@suse.cz> 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. > I'm attaching v3. > > Thanks, > Martin > >> >> Otherwise looks good to me. >> >> Richard. >> >> >>> Martin >>> >>>> >>>>> Thanks, >>>>> Martin >>>>> >>>>>> >>>>>> Richard. >>>>>> >>>>>>> Thanks, >>>>>>> Martin >>>>> >>> >
On Mon, Oct 26, 2015 at 2:48 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 22, 2015 at 1:02 PM, Martin Liška <mliska@suse.cz> wrote: >> On 10/21/2015 04:06 PM, Richard Biener wrote: >>> On Wed, Oct 21, 2015 at 1:24 PM, Martin Liška <mliska@suse.cz> wrote: >>>> On 10/21/2015 11:59 AM, Richard Biener wrote: >>>>> On Wed, Oct 21, 2015 at 11:19 AM, Martin Liška <mliska@suse.cz> wrote: >>>>>> On 10/20/2015 03:39 PM, Richard Biener wrote: >>>>>>> On Tue, Oct 20, 2015 at 3:00 PM, Martin Liška <mliska@suse.cz> 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'm giving that a shot now (removing push/pop_cfun in release_body) > > 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. > >> I'm attaching v3. >> >> Thanks, >> Martin >> >>> >>> Otherwise looks good to me. >>> >>> Richard. >>> >>> >>>> Martin >>>> >>>>> >>>>>> Thanks, >>>>>> Martin >>>>>> >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>>> Thanks, >>>>>>>> Martin >>>>>> >>>> >>
From d299b4c3c10432cda71c0aeb1becc9058ae818c1 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 22 Oct 2015 12:55:00 +0200 Subject: [PATCH] Pass manager: add support for termination of pass list gcc/ChangeLog: 2015-10-21 Martin Liska <mliska@suse.cz> * function.c (pop_cfun): Add new condition to checking assert. * passes.c (execute_one_pass): Handle TODO_discard_function. (execute_pass_list_1): Stop pass manager if a function has release body. (execute_pass_list): Free dominators info just for functions that have gimple body. * tree-pass.h (TODO_discard_function): Introduce new TODO. --- gcc/function.c | 1 + gcc/passes.c | 19 ++++++++++++++++++- gcc/tree-pass.h | 3 +++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/gcc/function.c b/gcc/function.c index f774214..a829d71 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4770,6 +4770,7 @@ pop_cfun (void) pop_cfun. */ gcc_checking_assert (in_dummy_function || !cfun + || !gimple_has_body_p (current_function_decl) || current_function_decl == cfun->decl); set_cfun (new_cfun); current_function_decl = new_cfun ? new_cfun->decl : NULL_TREE; diff --git a/gcc/passes.c b/gcc/passes.c index 6ef6d2e..692ea97 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -2383,6 +2383,20 @@ 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); + + cgraph_node::get (cfun->decl)->release_body (); + } + /* Signal this is a suitable GC collection point. */ if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect)) ggc_collect (); @@ -2397,6 +2411,9 @@ 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; if (execute_one_pass (pass) && pass->sub) execute_pass_list_1 (pass->sub); pass = pass->next; @@ -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); diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index c37e4b2..fd6d8ba 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -300,6 +300,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.0