From: Richard Biener <richard.guenther@gmail.com>
To: "Martin Liška" <mliska@suse.cz>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Pass manager: add support for termination of pass list
Date: Mon, 26 Oct 2015 13:49:00 -0000 [thread overview]
Message-ID: <CAFiYyc3GwQu58mmdEdWLGB3+iODREUC-KHBXhAWbz3+_4cJLNQ@mail.gmail.com> (raw)
In-Reply-To: <5628C262.8000205@suse.cz>
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
>>>>>
>>>
>
next prev parent reply other threads:[~2015-10-26 13:48 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-20 13:20 Martin Liška
2015-10-20 13:45 ` Richard Biener
2015-10-21 9:20 ` Martin Liška
2015-10-21 10:00 ` Richard Biener
2015-10-21 11:41 ` Martin Liška
2015-10-21 14:37 ` Richard Biener
2015-10-22 11:10 ` Martin Liška
2015-10-26 13:49 ` Richard Biener [this message]
2015-10-26 14:11 ` Richard Biener
2015-10-27 12:40 ` Martin Liška
2015-10-27 14:50 ` Richard Biener
2015-10-27 15:32 ` Martin Liška
2015-10-28 15:29 ` Richard Biener
2015-10-29 10:07 ` Martin Liška
2015-10-29 13:16 ` Richard Biener
2015-10-29 14:59 ` Martin Liška
2015-10-30 8:56 ` Richard Biener
2015-10-30 12:06 ` Martin Liška
2015-10-30 12:14 ` Richard Biener
2015-10-30 13:00 ` Martin Liška
2015-11-03 13:46 ` Richard Biener
2015-11-03 14:14 ` Martin Liška
2015-11-03 14:44 ` Richard Biener
2015-11-04 10:38 ` Martin Liška
2015-11-04 14:46 ` Richard Biener
2015-11-04 16:51 ` Martin Liška
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc3GwQu58mmdEdWLGB3+iODREUC-KHBXhAWbz3+_4cJLNQ@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=mliska@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).