On 15 September 2017 at 17:49, Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > On 1 September 2017 at 08:09, Prathamesh Kulkarni > <prathamesh.kulka...@linaro.org> wrote: >> On 17 August 2017 at 18:02, Prathamesh Kulkarni >> <prathamesh.kulka...@linaro.org> wrote: >>> On 8 August 2017 at 09:50, Prathamesh Kulkarni >>> <prathamesh.kulka...@linaro.org> wrote: >>>> On 31 July 2017 at 23:53, Prathamesh Kulkarni >>>> <prathamesh.kulka...@linaro.org> wrote: >>>>> On 23 May 2017 at 19:10, Prathamesh Kulkarni >>>>> <prathamesh.kulka...@linaro.org> wrote: >>>>>> On 19 May 2017 at 19:02, Jan Hubicka <hubi...@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 >> ping * 3 https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html > ping * 4 https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html Hi Honza, Could you please have a look at this patch ? https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html
I tested it with SPEC2006 on AArch64 Cortex-a57 processor and saw some improvement for 433.milc (+1.79%), 437.leslie3d (+2.84%) and 470.lbm (+4%) and not much differences for other benchmarks. I don't expect them to be precise though, it was run with only one iteration of SPEC. Thanks! Regards, Prathamesh > > Thanks, > Prathamesh >> >> Thanks, >> Prathamesh >>> >>> Thanks, >>> Prathamesh >>>> >>>> Thanks, >>>> Prathamesh >>>>> >>>>> Regards, >>>>> Prathamesh >>>>>> >>>>>> Thanks, >>>>>> Prathamesh >>>>>>> >>>>>>> Honza