On 10/27/2015 03:49 PM, Richard Biener wrote:
> On Tue, Oct 27, 2015 at 1:36 PM, Martin Liška <mli...@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 <mli...@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 <mli...@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 <mli...@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 <mli...@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
>>>>>>>>
>>>>>>
>>>>
>>
>From 9cefde12a9f52e402a0ab9a4b3b3077a34e7a38e Mon Sep 17 00:00:00 2001
From: marxin <mli...@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
