diff mbox

Pass manager: add support for termination of pass list

Message ID 562F6FC6.1020200@suse.cz
State New
Headers show

Commit Message

Martin Liška Oct. 27, 2015, 12:36 p.m. UTC 
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?
Thanks,
Martin

> 
>> I'm attaching v3.
>>
>> Thanks,
>> Martin
>>
>>>
>>> Otherwise looks good to me.
>>>
>>> Richard.
>>>
>>>
>>>> Martin
>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Martin
>>>>>>
>>>>
>>

Comments

Richard Biener Oct. 27, 2015, 2:49 p.m. UTC | #1 
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.

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

Richard.


> Thanks,
> Martin
>
>>
>>> I'm attaching v3.
>>>
>>> Thanks,
>>> Martin
>>>
>>>>
>>>> Otherwise looks good to me.
>>>>
>>>> Richard.
>>>>
>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Martin
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Martin
>>>>>>>
>>>>>
>>>
>
diff mbox

Patch

From 4d02a44cef73dbdced230b8b3a80183b9bcca264 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 22 Oct 2015 12:46:16 +0200
Subject: [PATCH] Version 4.

---
 gcc/function.c  |  7 +++++++
 gcc/hsa-gen.c   |  3 +--
 gcc/passes.c    | 26 +++++++++++++++++++++++++-
 gcc/tree-pass.h |  3 +++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/gcc/function.c b/gcc/function.c
index db5bc1c..6878e9d 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -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;
+    }
+
   struct function *new_cfun = cfun_stack.pop ();
   /* When in_dummy_function, we do have a cfun but current_function_decl is
      NULL.  We also allow pushing NULL cfun and subsequently changing
diff --git a/gcc/passes.c b/gcc/passes.c
index 8b3fb2f..87dd25f 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2378,6 +2378,26 @@  execute_one_pass (opt_pass *pass)
 
   current_pass = NULL;
 
+  if (todo_after & TODO_discard_function)
+    {
+      gcc_assert (cfun);
+      /* As cgraph_node::release_body expects release dominators info,
+	 we have to release it.  */
+      if (dom_info_available_p (CDI_DOMINATORS))
+	free_dominance_info (CDI_DOMINATORS);
+
+      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 ();
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2392,6 +2412,9 @@  execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (current_function_decl == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
 
@@ -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 ();
 }
 
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 7a5f476..2627df3 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -296,6 +296,9 @@  protected:
 /* Rebuild the callgraph edges.  */
 #define TODO_rebuild_cgraph_edges       (1 << 22)
 
+/* Release function body and stop pass manager.  */
+#define TODO_discard_function		(1 << 23)
+
 /* Internally used in execute_function_todo().  */
 #define TODO_update_ssa_any		\
     (TODO_update_ssa			\
-- 
2.6.2