Re: [PATCH] Pass manager: add support for termination of pass list

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Oct 26, 2015 at 2:48 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Oct 22, 2015 at 1:02 PM, Martin LiÅka <mliska@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 <mliska@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 <mliska@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 <mliska@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'm giving that a shot now (removing push/pop_cfun in release_body)

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