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.
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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
