Re: [RFC] propagate malloc attribute in ipa-pure-const pass

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

On 25 September 2017 at 17:24, Jan Hubicka <hubicka@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