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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

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?
Thanks,
Martin

[-- Attachment #2: 0001-Add-support-for-termination-of-pass-manager.patch --]
[-- Type: text/x-patch, Size: 3042 bytes --]

From fc60561c3ac09188fb61dac61b1cc2422061fc1d Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 19 Oct 2015 23:26:54 +0200
Subject: [PATCH] Add support for termination of pass manager.

gcc/ChangeLog:

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

	* passes.c (execute_one_pass): Add new argument called exit.
	(execute_pass_list_1): Terminate pass manager if a pass
	requests termination.
	(execute_ipa_pass_list): Likewise.
	* tree-pass.h: Introduce new TODO_stop_pass_execution.
---
 gcc/passes.c    | 24 ++++++++++++++++++++----
 gcc/tree-pass.h |  3 +++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/gcc/passes.c b/gcc/passes.c
index 6ef6d2e..1199ae3 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2279,10 +2279,11 @@ override_gate_status (opt_pass *pass, tree func, bool gate_status)
 }
 
 
-/* Execute PASS. */
+/* Execute PASS.  If the PASS requests to stop after its execution, EXIT
+   value is set to true.  */
 
 bool
-execute_one_pass (opt_pass *pass)
+execute_one_pass (opt_pass *pass, bool *exit)
 {
   unsigned int todo_after = 0;
 
@@ -2387,18 +2388,28 @@ execute_one_pass (opt_pass *pass)
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
 
+  /* If finish TODO flags contain TODO_stop_pass_execution, set exit = true.  */
+  if (todo_after & TODO_stop_pass_execution)
+    *exit = true;
+
   return true;
 }
 
 static void
 execute_pass_list_1 (opt_pass *pass)
 {
+  bool stop_pass_execution = false;
+
   do
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
-      if (execute_one_pass (pass) && pass->sub)
+      if (execute_one_pass (pass, &stop_pass_execution) && pass->sub)
         execute_pass_list_1 (pass->sub);
+
+      if (stop_pass_execution)
+	return;
+
       pass = pass->next;
     }
   while (pass);
@@ -2739,12 +2750,14 @@ ipa_read_optimization_summaries (void)
 void
 execute_ipa_pass_list (opt_pass *pass)
 {
+  bool stop_pass_execution = false;
+
   do
     {
       gcc_assert (!current_function_decl);
       gcc_assert (!cfun);
       gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
-      if (execute_one_pass (pass) && pass->sub)
+      if (execute_one_pass (pass, &stop_pass_execution) && pass->sub)
 	{
 	  if (pass->sub->type == GIMPLE_PASS)
 	    {
@@ -2763,6 +2776,9 @@ execute_ipa_pass_list (opt_pass *pass)
       gcc_assert (!current_function_decl);
       symtab->process_new_functions ();
       pass = pass->next;
+
+      if (stop_pass_execution)
+	return;
     }
   while (pass);
 }
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index c37e4b2..a481bac 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)
 
+/* Stop pass manager after execution of a pass.  */
+#define TODO_stop_pass_execution	(1 << 23)
+
 /* Internally used in execute_function_todo().  */
 #define TODO_update_ssa_any		\
     (TODO_update_ssa			\
-- 
2.6.0

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

[Richard Biener]

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.

Richard.

> Thanks,
> Martin

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

[Martin Liška]

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?

Thanks,
Martin

> 
> Richard.
> 
>> Thanks,
>> Martin

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

[Richard Biener]

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,
> Martin
>
>>
>> Richard.
>>
>>> Thanks,
>>> Martin
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

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.

Martin

> 
>> Thanks,
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>> Martin
>>


[-- Attachment #2: 0001-Pass-manager-add-support-for-termination-of-pass-lis.patch --]
[-- Type: text/x-patch, Size: 2989 bytes --]

From 3c969ee1257f88d9efaf6a55cf37705b847fa7ed Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 21 Oct 2015 13:18:29 +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..a4b2a68 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2387,6 +2387,19 @@ execute_one_pass (opt_pass *pass)
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
 
+  if (todo_after & TODO_discard_function)
+    {
+      /* 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 ();
+    }
+
   return true;
 }
 
@@ -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;
       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

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

[Richard Biener]

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

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

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

Otherwise looks good to me.

Richard.


> Martin
>
>>
>>> Thanks,
>>> Martin
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>> Martin
>>>
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

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


[-- Attachment #2: 0001-Pass-manager-add-support-for-termination-of-pass-lis.patch --]
[-- Type: text/x-patch, Size: 3074 bytes --]

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

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

[Richard Biener]

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.

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

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

[Richard Biener]

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]

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


[-- Attachment #2: 0001-Version-4.patch --]
[-- Type: text/x-patch, Size: 2898 bytes --]

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

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

[Richard Biener]

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 7616 bytes --]

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.

Thanks,
Martin

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


[-- Attachment #2: 0001-Version-5.patch --]
[-- Type: text/x-patch, Size: 2889 bytes --]

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

---
 gcc/function.c  |  7 +++++++
 gcc/hsa-gen.c   |  3 +--
 gcc/passes.c    | 28 +++++++++++++++++++++++++++-
 gcc/tree-pass.h |  3 +++
 4 files changed, 38 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..7b84d17 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2378,6 +2378,28 @@ 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);
+
+      tree fn = cfun->decl;
+
+      /* Pop function inserted in execute_pass_list.  */
+      pop_cfun ();
+
+      cgraph_node::get (fn)->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 +2414,9 @@ execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (cfun == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
 
@@ -2405,11 +2430,12 @@ execute_pass_list (function *fn, opt_pass *pass)
 {
   push_cfun (fn);
   execute_pass_list_1 (pass);
-  if (fn->cfg)
+  if (cfun && 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

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

[Richard Biener]

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.

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

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

[Martin Liška]

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.

Martin

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

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

[Richard Biener]

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.

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

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

[Martin Liška]

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?

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

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

[Richard Biener]

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

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

[Martin Liška]

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

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

[Richard Biener]

On Fri, Oct 30, 2015 at 12:59 PM, Martin Liška <mliska@suse.cz> wrote:
> 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

So I suggest to do the push/pop of cfun there.
do_per_function_toporder can be made static btw.

Richard.

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

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On 10/30/2015 01:13 PM, Richard Biener wrote:
> So I suggest to do the push/pop of cfun there.
> do_per_function_toporder can be made static btw.
> 
> Richard.

Right, I've done that and it works (bootstrap has been currently running),
feasible for HSA branch too.

tree-pass.h:

/* Declare for plugins.  */
extern void do_per_function_toporder (void (*) (function *, void *), void *);

Attaching the patch that I'm going to test.

Martin


[-- Attachment #2: 0001-Pass-manager-add-support-for-termination-of-pass-lis.patch --]
[-- Type: text/x-patch, Size: 3162 bytes --]

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

gcc/ChangeLog:

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

	* passes.c (do_per_function_toporder): Push to cfun before
	calling the pass manager.
	(execute_one_pass): Handle TODO_discard_function.
	(execute_pass_list_1): Terminate if current function is null.
	(execute_pass_list): Do not push and pop function.
	* tree-pass.h: Define new TODO_discard_function.
---
 gcc/passes.c    | 32 ++++++++++++++++++++++++++++----
 gcc/tree-pass.h |  3 +++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/gcc/passes.c b/gcc/passes.c
index 8b3fb2f..f24fc57 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1728,7 +1728,12 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data)
 	  order[i] = NULL;
 	  node->process = 0;
 	  if (node->has_gimple_body_p ())
-	    callback (DECL_STRUCT_FUNCTION (node->decl), data);
+	    {
+	      struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
+	      push_cfun (fn);
+	      callback (fn, data);
+	      pop_cfun ();
+	    }
 	}
       symtab->remove_cgraph_removal_hook (hook);
     }
@@ -2378,6 +2383,23 @@ 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 (current_function_decl)->release_body ();
+
+      current_function_decl = NULL;
+      set_cfun (NULL);
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2392,6 +2414,9 @@ execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (cfun == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
 
@@ -2403,14 +2428,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)
+  if (cfun && fn->cfg)
     {
       free_dominance_info (CDI_DOMINATORS);
       free_dominance_info (CDI_POST_DOMINATORS);
     }
-  pop_cfun ();
 }
 
 /* Write out all LTO data.  */
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

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

[Richard Biener]

On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
> On 10/30/2015 01:13 PM, Richard Biener wrote:
>> So I suggest to do the push/pop of cfun there.
>> do_per_function_toporder can be made static btw.
>>
>> Richard.
>
> Right, I've done that and it works (bootstrap has been currently running),
> feasible for HSA branch too.
>
> tree-pass.h:
>
> /* Declare for plugins.  */
> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>
> Attaching the patch that I'm going to test.

Err.

+      cgraph_node::get (current_function_decl)->release_body ();
+
+      current_function_decl = NULL;
+      set_cfun (NULL);

I'd have expected

      tree fn = cfun->decl;
      pop_cfun ();
      gcc_assert (!cfun);
      cgraph_node::get (fn)->release_body ();

here.

> Martin
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

On 11/03/2015 02:46 PM, Richard Biener wrote:
> On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 10/30/2015 01:13 PM, Richard Biener wrote:
>>> So I suggest to do the push/pop of cfun there.
>>> do_per_function_toporder can be made static btw.
>>>
>>> Richard.
>>
>> Right, I've done that and it works (bootstrap has been currently running),
>> feasible for HSA branch too.
>>
>> tree-pass.h:
>>
>> /* Declare for plugins.  */
>> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>>
>> Attaching the patch that I'm going to test.
> 
> Err.
> 
> +      cgraph_node::get (current_function_decl)->release_body ();
> +
> +      current_function_decl = NULL;
> +      set_cfun (NULL);
> 
> I'd have expected
> 
>       tree fn = cfun->decl;
>       pop_cfun ();
>       gcc_assert (!cfun);
>       cgraph_node::get (fn)->release_body ();
> 
> here.

Yeah, that works, but we have to add following hunk:

diff --git a/gcc/function.c b/gcc/function.c
index aaf49a4..4718fe1 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4756,6 +4756,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


If you are fine with that, looks we've fixed all issues related to the change, right?
Updated version of the is attached.

Martin

> 
>> Martin
>>


[-- Attachment #2: 0001-Pass-manager-add-support-for-termination-of-pass-lis.patch --]
[-- Type: text/x-patch, Size: 3814 bytes --]

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

gcc/ChangeLog:

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

	* function.c (pop_cfun): Set cfun and current_function_decl to
	NULL if the cfun_stack is empty.
	* passes.c (do_per_function_toporder): Push to cfun before
	calling the pass manager.
	(execute_one_pass): Handle TODO_discard_function.
	(execute_pass_list_1): Terminate if current function is null.
	(execute_pass_list): Do not push and pop function.
	* tree-pass.h: Define new TODO_discard_function.
---
 gcc/function.c  |  7 +++++++
 gcc/passes.c    | 32 ++++++++++++++++++++++++++++----
 gcc/tree-pass.h |  3 +++
 4 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/gcc/function.c b/gcc/function.c
index aaf49a4..4718fe1 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4756,6 +4756,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 d9af93a..d764a22 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1706,7 +1706,12 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data)
 	  order[i] = NULL;
 	  node->process = 0;
 	  if (node->has_gimple_body_p ())
-	    callback (DECL_STRUCT_FUNCTION (node->decl), data);
+	    {
+	      struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
+	      push_cfun (fn);
+	      callback (fn, data);
+	      pop_cfun ();
+	    }
 	}
       symtab->remove_cgraph_removal_hook (hook);
     }
@@ -2347,6 +2352,23 @@ 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);
+
+      tree fn = cfun->decl;
+      pop_cfun ();
+      gcc_assert (!cfun);
+      cgraph_node::get (fn)->release_body ();
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2361,6 +2383,9 @@ execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (cfun == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
 
@@ -2372,14 +2397,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)
+  if (cfun && fn->cfg)
     {
       free_dominance_info (CDI_DOMINATORS);
       free_dominance_info (CDI_POST_DOMINATORS);
     }
-  pop_cfun ();
 }
 
 /* Write out all LTO data.  */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index c03e3d8..713f068 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.2

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

[Richard Biener]

On Tue, Nov 3, 2015 at 3:13 PM, Martin Liška <mliska@suse.cz> wrote:
> On 11/03/2015 02:46 PM, Richard Biener wrote:
>> On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 10/30/2015 01:13 PM, Richard Biener wrote:
>>>> So I suggest to do the push/pop of cfun there.
>>>> do_per_function_toporder can be made static btw.
>>>>
>>>> Richard.
>>>
>>> Right, I've done that and it works (bootstrap has been currently running),
>>> feasible for HSA branch too.
>>>
>>> tree-pass.h:
>>>
>>> /* Declare for plugins.  */
>>> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>>>
>>> Attaching the patch that I'm going to test.
>>
>> Err.
>>
>> +      cgraph_node::get (current_function_decl)->release_body ();
>> +
>> +      current_function_decl = NULL;
>> +      set_cfun (NULL);
>>
>> I'd have expected
>>
>>       tree fn = cfun->decl;
>>       pop_cfun ();
>>       gcc_assert (!cfun);
>>       cgraph_node::get (fn)->release_body ();
>>
>> here.
>
> Yeah, that works, but we have to add following hunk:
>
> diff --git a/gcc/function.c b/gcc/function.c
> index aaf49a4..4718fe1 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -4756,6 +4756,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

Why again?  cfun should be set via push_cfun here so what's having cfun == NULL
at the pop_cfun point?  Or rather, what code used set_cfun () instead
of push_cfun ()?

>
> If you are fine with that, looks we've fixed all issues related to the change, right?
> Updated version of the is attached.
>
> Martin
>
>>
>>> Martin
>>>
>

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

On 11/03/2015 03:44 PM, Richard Biener wrote:
> On Tue, Nov 3, 2015 at 3:13 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 11/03/2015 02:46 PM, Richard Biener wrote:
>>> On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
>>>> On 10/30/2015 01:13 PM, Richard Biener wrote:
>>>>> So I suggest to do the push/pop of cfun there.
>>>>> do_per_function_toporder can be made static btw.
>>>>>
>>>>> Richard.
>>>>
>>>> Right, I've done that and it works (bootstrap has been currently running),
>>>> feasible for HSA branch too.
>>>>
>>>> tree-pass.h:
>>>>
>>>> /* Declare for plugins.  */
>>>> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>>>>
>>>> Attaching the patch that I'm going to test.
>>>
>>> Err.
>>>
>>> +      cgraph_node::get (current_function_decl)->release_body ();
>>> +
>>> +      current_function_decl = NULL;
>>> +      set_cfun (NULL);
>>>
>>> I'd have expected
>>>
>>>       tree fn = cfun->decl;
>>>       pop_cfun ();
>>>       gcc_assert (!cfun);
>>>       cgraph_node::get (fn)->release_body ();
>>>
>>> here.
>>
>> Yeah, that works, but we have to add following hunk:
>>
>> diff --git a/gcc/function.c b/gcc/function.c
>> index aaf49a4..4718fe1 100644
>> --- a/gcc/function.c
>> +++ b/gcc/function.c
>> @@ -4756,6 +4756,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
> 
> Why again?  cfun should be set via push_cfun here so what's having cfun == NULL
> at the pop_cfun point?  Or rather, what code used set_cfun () instead
> of push_cfun ()?
> 
>>
>> If you are fine with that, looks we've fixed all issues related to the change, right?
>> Updated version of the is attached.
>>
>> Martin
>>
>>>
>>>> Martin
>>>>
>>

Hi Richard.

There's the patch we talked about yesterday. It contains a few modification that
were necessary to make it working:

1) cgraph_node::expand_thunk uses just set_cfun (NULL) & current_function_decl = NULL because

      bb = then_bb = else_bb = return_bb
	= init_lowered_empty_function (thunk_fndecl, true, count);

in last branch of expand_thunk which calls allocate_struct_function.

2) i386.c and rs6000.c call init_function_start, as I move preamble to callers, I did the same
for these two functions

I've been running regression test and bootstrap.

Martin

[-- Attachment #2: 0001-Test.patch --]
[-- Type: text/x-patch, Size: 5957 bytes --]

From d0674bfa7d649d4a00b60da057c351b1328085c5 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 3 Nov 2015 16:28:36 +0100
Subject: [PATCH] Test.

---
 gcc/cgraphunit.c           | 10 ++++++----
 gcc/config/i386/i386.c     |  1 +
 gcc/config/rs6000/rs6000.c |  1 +
 gcc/function.c             |  5 -----
 gcc/passes.c               | 32 ++++++++++++++++++++++++++++----
 gcc/tree-pass.h            |  3 +++
 6 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 43d3185..f73d9a7 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1618,6 +1618,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       fn_block = make_node (BLOCK);
       BLOCK_VARS (fn_block) = a;
       DECL_INITIAL (thunk_fndecl) = fn_block;
+      allocate_struct_function (thunk_fndecl, false);
       init_function_start (thunk_fndecl);
       cfun->is_thunk = 1;
       insn_locations_init ();
@@ -1632,7 +1633,6 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       insn_locations_finalize ();
       init_insn_lengths ();
       free_after_compilation (cfun);
-      set_cfun (NULL);
       TREE_ASM_WRITTEN (thunk_fndecl) = 1;
       thunk.thunk_p = false;
       analyzed = false;
@@ -1944,9 +1944,11 @@ cgraph_node::expand (void)
   bitmap_obstack_initialize (NULL);
 
   /* Initialize the RTL code for the function.  */
-  current_function_decl = decl;
   saved_loc = input_location;
   input_location = DECL_SOURCE_LOCATION (decl);
+
+  gcc_assert (DECL_STRUCT_FUNCTION (decl));
+  push_cfun (DECL_STRUCT_FUNCTION (decl));
   init_function_start (decl);
 
   gimple_register_cfg_hooks ();
@@ -2014,8 +2016,8 @@ cgraph_node::expand (void)
 
   /* Make sure that BE didn't give up on compiling.  */
   gcc_assert (TREE_ASM_WRITTEN (decl));
-  set_cfun (NULL);
-  current_function_decl = NULL;
+  if (cfun)
+    pop_cfun ();
 
   /* It would make a lot more sense to output thunks before function body to get more
      forward and lest backwarding jumps.  This however would need solving problem
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 66024e2..2a965f6 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10958,6 +10958,7 @@ ix86_code_end (void)
 
       DECL_INITIAL (decl) = make_node (BLOCK);
       current_function_decl = decl;
+      allocate_struct_function (decl, false);
       init_function_start (decl);
       first_function_block_is_cold = false;
       /* Make sure unwind info is emitted for the thunk if needed.  */
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 271c3f9..8bdd646 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -34594,6 +34594,7 @@ rs6000_code_end (void)
 
   DECL_INITIAL (decl) = make_node (BLOCK);
   current_function_decl = decl;
+  allocate_struct_function (decl, false);
   init_function_start (decl);
   first_function_block_is_cold = false;
   /* Make sure unwind info is emitted for the thunk if needed.  */
diff --git a/gcc/function.c b/gcc/function.c
index aaf49a4..0d7cabc 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4957,11 +4957,6 @@ init_dummy_function_start (void)
 void
 init_function_start (tree subr)
 {
-  if (subr && DECL_STRUCT_FUNCTION (subr))
-    set_cfun (DECL_STRUCT_FUNCTION (subr));
-  else
-    allocate_struct_function (subr, false);
-
   /* Initialize backend, if needed.  */
   initialize_rtl ();
 
diff --git a/gcc/passes.c b/gcc/passes.c
index f87dcf4..7a10cb6 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1706,7 +1706,12 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data)
 	  order[i] = NULL;
 	  node->process = 0;
 	  if (node->has_gimple_body_p ())
-	    callback (DECL_STRUCT_FUNCTION (node->decl), data);
+	    {
+	      struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
+	      push_cfun (fn);
+	      callback (fn, data);
+	      pop_cfun ();
+	    }
 	}
       symtab->remove_cgraph_removal_hook (hook);
     }
@@ -2347,6 +2352,23 @@ 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);
+
+      tree fn = cfun->decl;
+      pop_cfun ();
+      gcc_assert (!cfun);
+      cgraph_node::get (fn)->release_body ();
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2361,6 +2383,9 @@ execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (cfun == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
       pass = pass->next;
@@ -2371,14 +2396,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)
+  if (cfun && fn->cfg)
     {
       free_dominance_info (CDI_DOMINATORS);
       free_dominance_info (CDI_POST_DOMINATORS);
     }
-  pop_cfun ();
 }
 
 /* Write out all LTO data.  */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index ba53cca..49e22a9 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.2

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

[Richard Biener]

On Wed, Nov 4, 2015 at 11:38 AM, Martin Liška <mliska@suse.cz> wrote:
> On 11/03/2015 03:44 PM, Richard Biener wrote:
>> On Tue, Nov 3, 2015 at 3:13 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 11/03/2015 02:46 PM, Richard Biener wrote:
>>>> On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>> On 10/30/2015 01:13 PM, Richard Biener wrote:
>>>>>> So I suggest to do the push/pop of cfun there.
>>>>>> do_per_function_toporder can be made static btw.
>>>>>>
>>>>>> Richard.
>>>>>
>>>>> Right, I've done that and it works (bootstrap has been currently running),
>>>>> feasible for HSA branch too.
>>>>>
>>>>> tree-pass.h:
>>>>>
>>>>> /* Declare for plugins.  */
>>>>> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>>>>>
>>>>> Attaching the patch that I'm going to test.
>>>>
>>>> Err.
>>>>
>>>> +      cgraph_node::get (current_function_decl)->release_body ();
>>>> +
>>>> +      current_function_decl = NULL;
>>>> +      set_cfun (NULL);
>>>>
>>>> I'd have expected
>>>>
>>>>       tree fn = cfun->decl;
>>>>       pop_cfun ();
>>>>       gcc_assert (!cfun);
>>>>       cgraph_node::get (fn)->release_body ();
>>>>
>>>> here.
>>>
>>> Yeah, that works, but we have to add following hunk:
>>>
>>> diff --git a/gcc/function.c b/gcc/function.c
>>> index aaf49a4..4718fe1 100644
>>> --- a/gcc/function.c
>>> +++ b/gcc/function.c
>>> @@ -4756,6 +4756,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
>>
>> Why again?  cfun should be set via push_cfun here so what's having cfun == NULL
>> at the pop_cfun point?  Or rather, what code used set_cfun () instead
>> of push_cfun ()?
>>
>>>
>>> If you are fine with that, looks we've fixed all issues related to the change, right?
>>> Updated version of the is attached.
>>>
>>> Martin
>>>
>>>>
>>>>> Martin
>>>>>
>>>
>
> Hi Richard.
>
> There's the patch we talked about yesterday. It contains a few modification that
> were necessary to make it working:
>
> 1) cgraph_node::expand_thunk uses just set_cfun (NULL) & current_function_decl = NULL because
>
>       bb = then_bb = else_bb = return_bb
>         = init_lowered_empty_function (thunk_fndecl, true, count);
>
> in last branch of expand_thunk which calls allocate_struct_function.
>
> 2) i386.c and rs6000.c call init_function_start, as I move preamble to callers, I did the same
> for these two functions
>
> I've been running regression test and bootstrap.

Looks good to me.

Thanks,
Richard.

> Martin

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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

On 11/04/2015 03:46 PM, Richard Biener wrote:
> On Wed, Nov 4, 2015 at 11:38 AM, Martin Liška <mliska@suse.cz> wrote:
>> On 11/03/2015 03:44 PM, Richard Biener wrote:
>>> On Tue, Nov 3, 2015 at 3:13 PM, Martin Liška <mliska@suse.cz> wrote:
>>>> On 11/03/2015 02:46 PM, Richard Biener wrote:
>>>>> On Fri, Oct 30, 2015 at 1:53 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>> On 10/30/2015 01:13 PM, Richard Biener wrote:
>>>>>>> So I suggest to do the push/pop of cfun there.
>>>>>>> do_per_function_toporder can be made static btw.
>>>>>>>
>>>>>>> Richard.
>>>>>>
>>>>>> Right, I've done that and it works (bootstrap has been currently running),
>>>>>> feasible for HSA branch too.
>>>>>>
>>>>>> tree-pass.h:
>>>>>>
>>>>>> /* Declare for plugins.  */
>>>>>> extern void do_per_function_toporder (void (*) (function *, void *), void *);
>>>>>>
>>>>>> Attaching the patch that I'm going to test.
>>>>>
>>>>> Err.
>>>>>
>>>>> +      cgraph_node::get (current_function_decl)->release_body ();
>>>>> +
>>>>> +      current_function_decl = NULL;
>>>>> +      set_cfun (NULL);
>>>>>
>>>>> I'd have expected
>>>>>
>>>>>       tree fn = cfun->decl;
>>>>>       pop_cfun ();
>>>>>       gcc_assert (!cfun);
>>>>>       cgraph_node::get (fn)->release_body ();
>>>>>
>>>>> here.
>>>>
>>>> Yeah, that works, but we have to add following hunk:
>>>>
>>>> diff --git a/gcc/function.c b/gcc/function.c
>>>> index aaf49a4..4718fe1 100644
>>>> --- a/gcc/function.c
>>>> +++ b/gcc/function.c
>>>> @@ -4756,6 +4756,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
>>>
>>> Why again?  cfun should be set via push_cfun here so what's having cfun == NULL
>>> at the pop_cfun point?  Or rather, what code used set_cfun () instead
>>> of push_cfun ()?
>>>
>>>>
>>>> If you are fine with that, looks we've fixed all issues related to the change, right?
>>>> Updated version of the is attached.
>>>>
>>>> Martin
>>>>
>>>>>
>>>>>> Martin
>>>>>>
>>>>
>>
>> Hi Richard.
>>
>> There's the patch we talked about yesterday. It contains a few modification that
>> were necessary to make it working:
>>
>> 1) cgraph_node::expand_thunk uses just set_cfun (NULL) & current_function_decl = NULL because
>>
>>       bb = then_bb = else_bb = return_bb
>>         = init_lowered_empty_function (thunk_fndecl, true, count);
>>
>> in last branch of expand_thunk which calls allocate_struct_function.
>>
>> 2) i386.c and rs6000.c call init_function_start, as I move preamble to callers, I did the same
>> for these two functions
>>
>> I've been running regression test and bootstrap.
> 
> Looks good to me.
> 
> Thanks,
> Richard.
> 
>> Martin

Thank you for review, installed as r229764. Final version of the patch
(Changelog entry has been added) is attached.

Martin

[-- Attachment #2: 0001-Pass-manager-add-support-for-termination-of-pass-lis.patch --]
[-- Type: text/x-patch, Size: 6754 bytes --]

From 825439772cd4d90c7253999f18457f49d23b37b4 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 3 Nov 2015 16:28:36 +0100
Subject: [PATCH] Pass manager: add support for termination of pass list

gcc/ChangeLog:

2015-11-04  Martin Liska  <mliska@suse.cz>

	* cgraphunit.c (cgraph_node::expand_thunk): Call
	allocate_struct_function before init_function_start.
	(cgraph_node::expand): Use push_cfun and pop_cfun.
	* config/i386/i386.c (ix86_code_end): Call
	allocate_struct_function before init_function_start.
	* config/rs6000/rs6000.c (rs6000_code_end): Likewise.
	* function.c (init_function_start): Move preamble to all
	callers.
	* passes.c (do_per_function_toporder): Use push_cfun and pop_cfun.
	(execute_one_pass): Handle newly added TODO_discard_function.
	(execute_pass_list_1): Terminate if cfun equals to NULL.
	(execute_pass_list): Do not push and pop cfun, expect that
	cfun is set.
	* tree-pass.h (TODO_discard_function): Define.
---
 gcc/cgraphunit.c           | 10 ++++++----
 gcc/config/i386/i386.c     |  1 +
 gcc/config/rs6000/rs6000.c |  1 +
 gcc/function.c             |  5 -----
 gcc/passes.c               | 32 ++++++++++++++++++++++++++++----
 gcc/tree-pass.h            |  3 +++
 6 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 43d3185..f73d9a7 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1618,6 +1618,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       fn_block = make_node (BLOCK);
       BLOCK_VARS (fn_block) = a;
       DECL_INITIAL (thunk_fndecl) = fn_block;
+      allocate_struct_function (thunk_fndecl, false);
       init_function_start (thunk_fndecl);
       cfun->is_thunk = 1;
       insn_locations_init ();
@@ -1632,7 +1633,6 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
       insn_locations_finalize ();
       init_insn_lengths ();
       free_after_compilation (cfun);
-      set_cfun (NULL);
       TREE_ASM_WRITTEN (thunk_fndecl) = 1;
       thunk.thunk_p = false;
       analyzed = false;
@@ -1944,9 +1944,11 @@ cgraph_node::expand (void)
   bitmap_obstack_initialize (NULL);
 
   /* Initialize the RTL code for the function.  */
-  current_function_decl = decl;
   saved_loc = input_location;
   input_location = DECL_SOURCE_LOCATION (decl);
+
+  gcc_assert (DECL_STRUCT_FUNCTION (decl));
+  push_cfun (DECL_STRUCT_FUNCTION (decl));
   init_function_start (decl);
 
   gimple_register_cfg_hooks ();
@@ -2014,8 +2016,8 @@ cgraph_node::expand (void)
 
   /* Make sure that BE didn't give up on compiling.  */
   gcc_assert (TREE_ASM_WRITTEN (decl));
-  set_cfun (NULL);
-  current_function_decl = NULL;
+  if (cfun)
+    pop_cfun ();
 
   /* It would make a lot more sense to output thunks before function body to get more
      forward and lest backwarding jumps.  This however would need solving problem
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 66024e2..2a965f6 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10958,6 +10958,7 @@ ix86_code_end (void)
 
       DECL_INITIAL (decl) = make_node (BLOCK);
       current_function_decl = decl;
+      allocate_struct_function (decl, false);
       init_function_start (decl);
       first_function_block_is_cold = false;
       /* Make sure unwind info is emitted for the thunk if needed.  */
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 271c3f9..8bdd646 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -34594,6 +34594,7 @@ rs6000_code_end (void)
 
   DECL_INITIAL (decl) = make_node (BLOCK);
   current_function_decl = decl;
+  allocate_struct_function (decl, false);
   init_function_start (decl);
   first_function_block_is_cold = false;
   /* Make sure unwind info is emitted for the thunk if needed.  */
diff --git a/gcc/function.c b/gcc/function.c
index aaf49a4..0d7cabc 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4957,11 +4957,6 @@ init_dummy_function_start (void)
 void
 init_function_start (tree subr)
 {
-  if (subr && DECL_STRUCT_FUNCTION (subr))
-    set_cfun (DECL_STRUCT_FUNCTION (subr));
-  else
-    allocate_struct_function (subr, false);
-
   /* Initialize backend, if needed.  */
   initialize_rtl ();
 
diff --git a/gcc/passes.c b/gcc/passes.c
index f87dcf4..7a10cb6 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1706,7 +1706,12 @@ do_per_function_toporder (void (*callback) (function *, void *data), void *data)
 	  order[i] = NULL;
 	  node->process = 0;
 	  if (node->has_gimple_body_p ())
-	    callback (DECL_STRUCT_FUNCTION (node->decl), data);
+	    {
+	      struct function *fn = DECL_STRUCT_FUNCTION (node->decl);
+	      push_cfun (fn);
+	      callback (fn, data);
+	      pop_cfun ();
+	    }
 	}
       symtab->remove_cgraph_removal_hook (hook);
     }
@@ -2347,6 +2352,23 @@ 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);
+
+      tree fn = cfun->decl;
+      pop_cfun ();
+      gcc_assert (!cfun);
+      cgraph_node::get (fn)->release_body ();
+    }
+
   /* Signal this is a suitable GC collection point.  */
   if (!((todo_after | pass->todo_flags_finish) & TODO_do_not_ggc_collect))
     ggc_collect ();
@@ -2361,6 +2383,9 @@ execute_pass_list_1 (opt_pass *pass)
     {
       gcc_assert (pass->type == GIMPLE_PASS
 		  || pass->type == RTL_PASS);
+
+      if (cfun == NULL)
+	return;
       if (execute_one_pass (pass) && pass->sub)
         execute_pass_list_1 (pass->sub);
       pass = pass->next;
@@ -2371,14 +2396,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)
+  if (cfun && fn->cfg)
     {
       free_dominance_info (CDI_DOMINATORS);
       free_dominance_info (CDI_POST_DOMINATORS);
     }
-  pop_cfun ();
 }
 
 /* Write out all LTO data.  */
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index ba53cca..49e22a9 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.2