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

[Jan Hubicka <hubicka at ucw dot cz>]

> 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.

+/* 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.

+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.

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