On Tue, Oct 27, 2015 at 1:36 PM, Martin Liška <[email protected]> wrote:
> On 10/26/2015 02:48 PM, Richard Biener wrote:
>> On Thu, Oct 22, 2015 at 1:02 PM, Martin Liška <[email protected]> wrote:
>>> On 10/21/2015 04:06 PM, Richard Biener wrote:
>>>> On Wed, Oct 21, 2015 at 1:24 PM, Martin Liška <[email protected]> wrote:
>>>>> On 10/21/2015 11:59 AM, Richard Biener wrote:
>>>>>> On Wed, Oct 21, 2015 at 11:19 AM, Martin Liška <[email protected]> wrote:
>>>>>>> On 10/20/2015 03:39 PM, Richard Biener wrote:
>>>>>>>> On Tue, Oct 20, 2015 at 3:00 PM, Martin Liška <[email protected]> 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.
+ 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?
Richard.
> Thanks,
> Martin
>
>>
>>> I'm attaching v3.
>>>
>>> Thanks,
>>> Martin
>>>
>>>>
>>>> Otherwise looks good to me.
>>>>
>>>> Richard.
>>>>
>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Martin
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Martin
>>>>>>>
>>>>>
>>>
>
