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