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

Martin Liška mliska@suse.cz
Fri Oct 30 12:06:00 GMT 2015


On 10/30/2015 09:54 AM, Richard Biener wrote:
> 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.

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

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



More information about the Gcc-patches mailing list