On 25 September 2017 at 17:24, Jan Hubicka <hubi...@ucw.cz> wrote: >> Hi Honza, >> Could you please have a look at this patch ? >> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02063.html > > I can and I should have done long time ago. I really apologize for slow > response > and I will try to be more timely from now on. The reason was that I had some > patches that I was thinking I would like to push out first, but I guess since > they are still not ready it is better to go other way around. No worries, and thanks for the feedback! > > +/* 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; > > Extra * at the beggining of line. It would make more sense to put those > and the other bits into function_summary rather than using the hooks > but that is something we co do incrementally. > > I wonder what happens here when, say, ipa-icf redirect the call to eqivaelnt > function and removes the callee? Perhaps we realy want to have set of call > sites rahter than nodes stored from analysis to execution. Call sites have > unique stmts and uids, so it will be possible to map them back and forth. IIUC, call site is represented using cgraph_edge ? So change return_callees_map to be the mapping from node to subset of it's call-sites where node returns the value of one of it's callees: static hash_map< cgraph_node *, vec<cgraph_edge *> *> *return_callees_map; ? > > +static bool > +check_retval_uses (tree retval, gimple *stmt) > +{ > > there is missing toplevel comment on those. > > +/* > + * Currently this function does a very conservative analysis to check if > + * function could be a malloc candidate. > + * > + * The function is considered to be a candidate if > + * 1) The function returns a value of pointer type. > + * 2) SSA_NAME_DEF_STMT (return_value) is either a function call or > + * a phi, and element of phi is either NULL or > + * SSA_NAME_DEF_STMT(element) is function call. > + * 3) The return-value has immediate uses only within comparisons (gcond or > gassign) > + * and return_stmt (and likewise a phi arg has immediate use only within > comparison > + * or the phi stmt). > + */ > > Now * in begginig of lines. Theoretically by coding standards the comment > should start with description of what function does and what are the > parameters. > I believe Richi already commented on this part - which is more of his domain, > but it seems fine to me. > > Pehraps with -details dump it would be nice to dump reason why the malloc > candidate was rejected. > > +DEBUG_FUNCTION > +static void > +dump_malloc_lattice (FILE *dump_file, const char *s) > > +static void > +propagate_malloc (void) > > For coding standards, please add block comments. Thanks for the suggestions, I will try to address them in the next version of the patch.
Regards, Prathamesh > > With these changes the patch looks good to me! > Honza > >> >> 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
