[PATCH] Support multi-versioning on self-recursive function (ipa/92133)

[Feng Xue OS]

Martin,

    Thanks for your review. I updated the patch with your comments.

Feng
---
2019-11-15  Feng Xue <fxue@os.amperecomputing.com>

	PR ipa/92133
	* doc/invoke.texi (ipa-cp-max-recursive-depth): Document new option.
	(ipa-cp-min-recursive-probability): Likewise.
	* params.opt (ipa-cp-max-recursive-depth): New.
	(ipa-cp-min-recursive-probability): Likewise.
	* ipa-cp.c (ipcp_lattice<valtype>::add_value): Add two new parameters
	val_p and unlimited.
	(self_recursively_generated_p): New function.
	(get_val_across_arith_op): Likewise.
	(propagate_vals_across_arith_jfunc): Add constant propagation for
	self-recursive function.
	(incorporate_penalties): Do not penalize pure self-recursive function.
	(good_cloning_opportunity_p): Dump node_is_self_scc flag.
	(propagate_constants_topo): Set node_is_self_scc flag for cgraph node.
	(get_info_about_necessary_edges): Relax hotness check for edge to
	self-recursive function.
	* ipa-prop.h (ipa_node_params): Add new field node_is_self_scc.
---

> The least important comment: Thanks for providing the ChangeLog but
> sending ChangeLog in the patch itself makes it difficult for people to
> apply the patch because the ChangeLog hunks will never apply cleanly.
> That's why people send them in the email body when they email
> gcc-patches.  Hopefully we'll be putting ChangeLogs only into the commit
> message soon and all of this will be a non-issue.
Ok.

>> +  if (val_pos_p)
>> +    {
>> +      for (val = values; val && val != *val_pos_p; val = val->next);

> Please put empty statements on a separate line (I think there is one
> more in the patch IIRC).
Done.

>> +  if (val_pos_p)
>> +    {
>> +      val->next = (*val_pos_p)->next;
>> +      (*val_pos_p)->next = val;
>> +      *val_pos_p = val;
>> +    }

> I must say I am not a big fan of the val_pos_p parameter.  Would simply
> putting new values always at the end of the list work to reduce the
> iterations?  At this point the function has traversed all the values
> already when searching if it is present, so it can remember the last
> one and just add stuff after it.
We need a parameter to record address of the added value, which will be used
in constructing next one. To place new value at the end of list is a good idea,
thus meaning of val_pos_p becomes simpler, it is only an out parameter now.

>> +      if (!src->val || cs->caller != cs->callee->function_symbol ()
>> +       || src->val == val)
>> +     return false;

> I'd suggest putting the condition calling function_symbol last.
Original condition order can reduce unnecessary evaluations on src->val==val,
which is false in most situation, while cs->caller != cs->callee->function_symbol ()
is true. 

>> +      for (src_val = src_lat->values; src_val && src_val != val;
>> +        src_val = src_val->next);

> I think that GNU code style dictates that when a for does not fit on a
> single line, the three bits have to be on a separate line each.  Plus
> please put the empty statement on its own line too.
Done.

>> +static tree
>> +get_val_across_arith_op (enum tree_code opcode,
>> +                      tree opnd1_type,
>> +                      tree opnd2,
>> +                      ipcp_value<tree> *src_val,
>> +                      tree res_type)

> The function should get at least a brief comment.
Done.

>> +           for (ipcp_value_source<tree> *s = src_val->sources; s;
>> +                s = s->next)

> I'm afraid you'll have to reformat this for loop too.
Done.
>> +     if (self_recursively_generated_p (src_val))
>> +       continue;

> I think you are missing a necessary call to
> dest_lat->set_contains_variable() here.  Either call it or initialize
> cstval to zero and call get_val_across_arith_op only when
> self_recursively_generated_p returns false;
Yes, this is a bug. Fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Enable-recursive-function-versioning.patch
Type: application/octet-stream
Size: 15207 bytes
Desc: 0001-Enable-recursive-function-versioning.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191125/4398a54a/attachment.obj>