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.
Martin
>
>> Thanks,
>> Martin
>>
>>>
>>> Richard.
>>>
>>>
>>>> Thanks,
>>>> Martin
>>>>
>>>>>
>>>>>> I'm attaching v3.
>>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>>>
>>>>>>> Otherwise looks good to me.
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>
>>>>>>>> Martin
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Martin
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Richard.
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Martin
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
