On Fri, Oct 30, 2015 at 12:59 PM, Martin Liška <mli...@suse.cz> wrote:
> 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
So I suggest to do the push/pop of cfun there.
do_per_function_toporder can be made static btw.
Richard.
> 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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
