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

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

On Thu, Oct 29, 2015 at 3:50 PM, Martin LiÅka <mliska@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 <mliska@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 <mliska@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 <mliska@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 <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 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
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>