On 10/30/2015 09:54 AM, Richard Biener wrote: > On Thu, Oct 29, 2015 at 3:50 PM, Martin Liška <mli...@suse.cz> wrote: >> On 10/29/2015 02:15 PM, Richard Biener wrote: >>> On Thu, Oct 29, 2015 at 10:49 AM, Martin Liška <mli...@suse.cz> wrote: >>>> On 10/28/2015 04:23 PM, Richard Biener wrote: >>>>> On Tue, Oct 27, 2015 at 4:30 PM, Martin Liška <mli...@suse.cz> wrote: >>>>>> On 10/27/2015 03:49 PM, Richard Biener wrote: >>>>>>> On Tue, Oct 27, 2015 at 1:36 PM, Martin Liška <mli...@suse.cz> wrote: >>>>>>>> On 10/26/2015 02:48 PM, Richard Biener wrote: >>>>>>>>> On Thu, Oct 22, 2015 at 1:02 PM, Martin Liška <mli...@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 <mli...@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 <mli...@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 <mli...@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. >>>>>>>> >>>>>>>> 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. >>> >>> popping it once should have that effect already. If not then go >>> figure why it does not. >> >> Hello. >> >> So the problem is that init_function_start calls set_cfun: >> >> #0 set_cfun (new_cfun=0x7ffff66efbd0) at ../../gcc/function.c:4740 >> #1 0x0000000000a500b8 in init_function_start (subr=0x7ffff66ec1c0) at >> ../../gcc/function.c:4972 >> #2 0x0000000000911ee3 in cgraph_node::expand (this=0x7ffff6a0ee60) at >> ../../gcc/cgraphunit.c:1970 >> >> So that first popping removes the function set in execute_pass_list and >> after that the stack is empty, >> but cfun is set to 0x7ffff66efbd0. >> >> Should I replace the second pop with set_cfun(NULL) or is it acceptable to >> call the popping for >> second time? > > Hmm, does > > @@ -2397,14 +2400,13 @@ execute_pass_list_1 (opt_pass *pass) > void > execute_pass_list (function *fn, opt_pass *pass) > { > - push_cfun (fn); > + gcc_assert (fn == cfun); > execute_pass_list_1 (pass); > if (fn->cfg) > { > free_dominance_info (CDI_DOMINATORS); > free_dominance_info (CDI_POST_DOMINATORS); > } > - pop_cfun (); > } > > /* Write out all LTO data. */ > > work? All callers seem to pass cfun as fun.
There's unfortunately situation where cfun == NULL: #0 execute_pass_list (fn=0x7ffff6abb348, pass=0x24512a0) at ../../gcc/passes.c:2428 #1 0x0000000000c7758d in do_per_function_toporder (callback=0xc78f1f <execute_pass_list(function*, opt_pass*)>, data=0x24512a0) at ../../gcc/passes.c:1731 #2 0x0000000000c79b41 in execute_ipa_pass_list (pass=0x2451240) at ../../gcc/passes.c:2771 #3 0x0000000000912a93 in ipa_passes () at ../../gcc/cgraphunit.c:2259 #4 0x0000000000912f07 in symbol_table::compile (this=0x7ffff68d30a8) at ../../gcc/cgraphunit.c:2400 Martin > > >> Thanks, >> Martin >> >> >>> >>> Richard. >>> >>>> Martin >>>> >>>>> >>>>>> Thanks, >>>>>> Martin >>>>>> >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>> >>>>>>>> Thanks, >>>>>>>> Martin >>>>>>>> >>>>>>>>> >>>>>>>>>> I'm attaching v3. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Martin >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Otherwise looks good to me. >>>>>>>>>>> >>>>>>>>>>> Richard. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Martin >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>> Martin >>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Richard. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>> Martin >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >>>> >>