Hi, Richard, This patch is a not bugfix, while it is small. Martin and Honza are fine with it. But now we are in stage 3, is it ok to commit?
Thanks,
Feng
________________________________________
From: Feng Xue OS <f...@os.amperecomputing.com>
Sent: Monday, November 25, 2019 10:17 PM
To: Martin Jambor; Jan Hubicka
Cc: luoxhu; gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Support multi-versioning on self-recursive function
(ipa/92133)
Martin,
Thanks for your review. I updated the patch with your comments.
Feng
---
2019-11-15 Feng Xue <f...@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.
0001-Enable-recursive-function-versioning.patch
Description: 0001-Enable-recursive-function-versioning.patch
