This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

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

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

> 
> @@ -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 attaching v3.

Thanks,
Martin

> 
> Otherwise looks good to me.
> 
> Richard.
> 
> 
>> Martin
>>
>>>
>>>> Thanks,
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>> Martin
>>>>
>>

>From d299b4c3c10432cda71c0aeb1becc9058ae818c1 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 22 Oct 2015 12:55:00 +0200
Subject: [PATCH] Pass manager: add support for termination of pass list

gcc/ChangeLog:

2015-10-21  Martin Liska  <mliska@suse.cz>

	* function.c (pop_cfun): Add new condition to checking assert.
	* passes.c (execute_one_pass): Handle TODO_discard_function.
	(execute_pass_list_1): Stop pass manager if a function
	has release body.
	(execute_pass_list): Free dominators info just for functions
	that have gimple body.
	* tree-pass.h (TODO_discard_function): Introduce new TODO.
---
 gcc/function.c  |  1 +
 gcc/passes.c    | 19 ++++++++++++++++++-
 gcc/tree-pass.h |  3 +++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/gcc/function.c b/gcc/function.c
index f774214..a829d71 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4770,6 +4770,7 @@ pop_cfun (void)
      pop_cfun.  */
   gcc_checking_assert (in_dummy_function
 		       || !cfun
+		       || !gimple_has_body_p (current_function_decl)
 		       || current_function_decl == cfun->decl);
   set_cfun (new_cfun);
   current_function_decl = new_cfun ? new_cfun->decl : NULL_TREE;
diff --git a/gcc/passes.c b/gcc/passes.c
index 6ef6d2e..692ea97 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2383,6 +2383,20 @@ 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);
+
+      cgraph_node::get (cfun->decl)->release_body ();
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2397,6 +2411,9 @@ 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;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
       pass = pass->next;
@@ -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);
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index c37e4b2..fd6d8ba 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -300,6 +300,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.0


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]