From: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
To: Jan Hubicka <hubicka@ucw.cz>
Cc: gcc Patches <gcc-patches@gcc.gnu.org>,
Richard Biener <rguenther@suse.de>
Subject: Re: [RFC] propagate malloc attribute in ipa-pure-const pass
Date: Thu, 17 Aug 2017 12:55:00 -0000 [thread overview]
Message-ID: <CAAgBjMk1cBaacnrikMmwocHkaHS2mG57BvY667r_-7TzKMo0LA@mail.gmail.com> (raw)
In-Reply-To: <CAAgBjMkrqebvROy_5SjQX5mV-gL+_JY6-q9O1Qh52LE6mSV4rA@mail.gmail.com>
On 8 August 2017 at 09:50, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 31 July 2017 at 23:53, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>> On 23 May 2017 at 19:10, Prathamesh Kulkarni
>> <prathamesh.kulkarni@linaro.org> wrote:
>>> On 19 May 2017 at 19:02, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>>>
>>>>> * LTO and memory management
>>>>> This is a general question about LTO and memory management.
>>>>> IIUC the following sequence takes place during normal LTO:
>>>>> LGEN: generate_summary, write_summary
>>>>> WPA: read_summary, execute ipa passes, write_opt_summary
>>>>>
>>>>> So I assumed it was OK in LGEN to allocate return_callees_map in
>>>>> generate_summary and free it in write_summary and during WPA, allocate
>>>>> return_callees_map in read_summary and free it after execute (since
>>>>> write_opt_summary does not require return_callees_map).
>>>>>
>>>>> However with fat LTO, it seems the sequence changes for LGEN with
>>>>> execute phase takes place after write_summary. However since
>>>>> return_callees_map is freed in pure_const_write_summary and
>>>>> propagate_malloc() accesses it in execute stage, it results in
>>>>> segmentation fault.
>>>>>
>>>>> To work around this, I am using the following hack in pure_const_write_summary:
>>>>> // FIXME: Do not free if -ffat-lto-objects is enabled.
>>>>> if (!global_options.x_flag_fat_lto_objects)
>>>>> free_return_callees_map ();
>>>>> Is there a better approach for handling this ?
>>>>
>>>> I think most passes just do not free summaries with -flto. We probably want
>>>> to fix it to make it possible to compile multiple units i.e. from plugin by
>>>> adding release_summaries method...
>>>> So I would say it is OK to do the same as others do and leak it with -flto.
>>>>> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
>>>>> index e457166ea39..724c26e03f6 100644
>>>>> --- a/gcc/ipa-pure-const.c
>>>>> +++ b/gcc/ipa-pure-const.c
>>>>> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see
>>>>> #include "tree-scalar-evolution.h"
>>>>> #include "intl.h"
>>>>> #include "opts.h"
>>>>> +#include "ssa.h"
>>>>>
>>>>> /* Lattice values for const and pure functions. Everything starts out
>>>>> being const, then may drop to pure and then neither depending on
>>>>> @@ -69,6 +70,15 @@ enum pure_const_state_e
>>>>>
>>>>> const char *pure_const_names[3] = {"const", "pure", "neither"};
>>>>>
>>>>> +enum malloc_state_e
>>>>> +{
>>>>> + PURE_CONST_MALLOC_TOP,
>>>>> + PURE_CONST_MALLOC,
>>>>> + PURE_CONST_MALLOC_BOTTOM
>>>>> +};
>>>>
>>>> It took me a while to work out what PURE_CONST means here :)
>>>> I would just call it something like STATE_MALLOC_TOP... or so.
>>>> ipa_pure_const is outdated name from the time pass was doing only
>>>> those two.
>>>>> @@ -109,6 +121,10 @@ typedef struct funct_state_d * funct_state;
>>>>>
>>>>> static vec<funct_state> funct_state_vec;
>>>>>
>>>>> +/* A map from node to subset of callees. The subset contains those callees
>>>>> + * whose return-value is returned by the node. */
>>>>> +static hash_map< cgraph_node *, vec<cgraph_node *>* > *return_callees_map;
>>>>> +
>>>>
>>>> Hehe, a special case of return jump function. We ought to support those more generally.
>>>> How do you keep it up to date over callgraph changes?
>>>>> @@ -921,6 +1055,23 @@ end:
>>>>> if (TREE_NOTHROW (decl))
>>>>> l->can_throw = false;
>>>>>
>>>>> + if (ipa)
>>>>> + {
>>>>> + vec<cgraph_node *> v = vNULL;
>>>>> + l->malloc_state = PURE_CONST_MALLOC_BOTTOM;
>>>>> + if (DECL_IS_MALLOC (decl))
>>>>> + l->malloc_state = PURE_CONST_MALLOC;
>>>>> + else if (malloc_candidate_p (DECL_STRUCT_FUNCTION (decl), v))
>>>>> + {
>>>>> + l->malloc_state = PURE_CONST_MALLOC_TOP;
>>>>> + vec<cgraph_node *> *callees_p = new vec<cgraph_node *> (vNULL);
>>>>> + for (unsigned i = 0; i < v.length (); ++i)
>>>>> + callees_p->safe_push (v[i]);
>>>>> + return_callees_map->put (fn, callees_p);
>>>>> + }
>>>>> + v.release ();
>>>>> + }
>>>>> +
>>>>
>>>> I would do non-ipa variant, too. I think most attributes can be detected that way
>>>> as well.
>>>>
>>>> The patch generally makes sense to me. It would be nice to make it easier to write such
>>>> a basic propagators across callgraph (perhaps adding a template doing the basic
>>>> propagation logic). Also I think you need to solve the problem with keeping your
>>>> summaries up to date across callgraph node removal and duplications.
>>> Thanks for the suggestions, I will try to address them in a follow-up patch.
>>> IIUC, I would need to modify ipa-pure-const cgraph hooks -
>>> add_new_function, remove_node_data, duplicate_node_data
>>> to keep return_callees_map up-to-date across callgraph node insertions
>>> and removal ?
>>>
>>> Also, if instead of having a separate data-structure like return_callees_map,
>>> should we rather have a flag within cgraph_edge, which marks that the
>>> caller may return the value of the callee ?
>> Hi,
>> Sorry for the very late response. I have attached an updated version
>> of the prototype patch,
>> which adds a non-ipa variant, and keeps return_callees_map up-to-date
>> across callgraph
>> node insertions and removal. For the non-ipa variant,
>> malloc_candidate_p() additionally checks
>> that all the "return callees" have DECL_IS_MALLOC set to true.
>> Bootstrapped+tested and LTO bootstrapped+tested on x86_64-unknown-linux-gnu.
>> Does it look OK so far ?
>>
>> Um sorry for this silly question, but I don't really understand how
>> does indirect call propagation
>> work in ipa-pure-const ? For example consider propagation of nothrow
>> attribute in following
>> test-case:
>>
>> __attribute__((noinline, noclone, nothrow))
>> int f1(int k) { return k; }
>>
>> __attribute__((noinline, noclone))
>> static int foo(int (*p)(int))
>> {
>> return p(10);
>> }
>>
>> __attribute__((noinline, noclone))
>> int bar(void)
>> {
>> return foo(f1);
>> }
>>
>> Shouldn't foo and bar be also marked as nothrow ?
>> Since foo indirectly calls f1 which is nothrow and bar only calls foo ?
>> The local-pure-const2 dump shows function is locally throwing for
>> "foo" and "bar".
>>
>> Um, I was wondering how to get "points-to" analysis for function-pointers,
>> to get list of callees that may be indirectly called from that
>> function pointer ?
>> In the patch I just set node to bottom if it contains indirect calls
>> which is far from ideal :(
>> I would be much grateful for suggestions on how to handle indirect calls.
>> Thanks!
> ping https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html
ping * 2 https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html
Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
>>
>> Regards,
>> Prathamesh
>>>
>>> Thanks,
>>> Prathamesh
>>>>
>>>> Honza
next prev parent reply other threads:[~2017-08-17 12:32 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-15 10:56 Prathamesh Kulkarni
2017-05-16 1:14 ` Jeff Law
2017-05-16 11:48 ` Richard Biener
2017-05-17 21:22 ` Martin Sebor
2017-05-18 7:07 ` Richard Biener
2017-05-19 13:18 ` Jan Hubicka
2017-05-19 13:34 ` Jan Hubicka
2017-05-23 13:48 ` Prathamesh Kulkarni
2017-07-31 18:23 ` Prathamesh Kulkarni
2017-08-08 4:21 ` Prathamesh Kulkarni
2017-08-17 12:55 ` Prathamesh Kulkarni [this message]
2017-09-01 2:39 ` Prathamesh Kulkarni
2017-09-15 12:19 ` Prathamesh Kulkarni
2017-09-25 18:13 ` Prathamesh Kulkarni
2017-09-26 0:24 ` Jan Hubicka
2017-09-27 1:11 ` Prathamesh Kulkarni
2017-09-29 19:28 ` Jan Hubicka
2017-10-06 2:16 ` Prathamesh Kulkarni
2017-10-06 13:04 ` Jan Hubicka
2017-10-07 1:46 ` Prathamesh Kulkarni
2017-10-07 19:35 ` Jan Hubicka
2017-10-07 22:17 ` Prathamesh Kulkarni
2017-10-13 23:34 ` Prathamesh Kulkarni
2017-10-23 9:37 ` Prathamesh Kulkarni
2017-10-24 10:57 ` Jan Hubicka
2017-10-25 11:18 ` Prathamesh Kulkarni
2017-10-25 15:26 ` Jan Hubicka
2017-10-27 10:52 ` Prathamesh Kulkarni
2017-10-27 12:20 ` Jan Hubicka
2017-10-27 12:44 ` Jan Hubicka
2017-10-27 13:00 ` Richard Biener
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=CAAgBjMk1cBaacnrikMmwocHkaHS2mG57BvY667r_-7TzKMo0LA@mail.gmail.com \
--to=prathamesh.kulkarni@linaro.org \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=rguenther@suse.de \
/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).