Re: [PATCH V2] Generalized value pass-through for self-recursive function (ipa/pr93203)

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Sat, Jan 25 2020, Feng Xue OS wrote:
> Made some changes.
>
> Feng
>
> ________________________________________
> From: Feng Xue OS
> Sent: Saturday, January 25, 2020 5:54 PM
> To: mjambor@suse.cz; Jan Hubicka; gcc-patches@gcc.gnu.org
> Subject: [PATCH] Generalized value pass-through for self-recursive function (ipa/pr93203)
>
> Besides simple pass-through (aggregate) jump function, arithmetic (aggregate)
> jump function could also bring same (aggregate) value as parameter passed-in
> for self-feeding recursive call.  For example,
>
>       f1 (int i)    /*  normal jump function */
>          {
>             f1 (i & 1);
>          }
>
> Suppose i is 0, recursive propagation via (i & 1) also gets 0, which
> can be seen as a simple pass-through of i.
>
>       f2 (int *p)  /* aggregate jump function */
>          {
>             int t = *p & 1;
>             f2 (&t);
>          }
> Likewise, if *p is 0, (*p & 1) is also 0, and &t is an aggregate simple
> pass-through of p.
>
> This patch is to support these two kinds of value pass-through.
> Bootstrapped/regtested on x86_64-linux and aarch64-linux.

sorry for the delay in the review.  As far as I am concerned, I am OK
with the patch but please see the few comments below:

> ---
> 2020-01-25  Feng Xue  <fxue@os.amperecomputing.com>
>
>         PR ipa/93203
>         * ipa-cp.c (ipcp_lattice::add_value): Add source with same call
>         edge but different source value.
>         (adjust_callers_for_value_intersection): New function.
>         (gather_edges_for_value): Adjust order of callers to let a
>         non-self-recursive caller be the first element.
>         (self_recursive_pass_through_p): Add a new parameter simple, and
>         check generalized self-recursive pass-through jump function.
>         (self_recursive_agg_pass_through_p): Likewise.
>         (find_more_scalar_values_for_callers_subset): Compute value from
>         pass-through jump function for self-recursive.
>         (intersect_with_plats): Remove code of itersection with unknown
>         place holder value.
>         (intersect_with_agg_replacements): Likewise.
>         (intersect_aggregates_with_edge): Deduce with from pass-through
>         jump function for self-recursive.
>         (decide_whether_version_node): Remove dead callers and adjust
>         order to let a non-self-recursive caller be the first element.
>
> From 74aef0cd2f40ff828a4b2abcbbdbbf4b1aea1fcf Mon Sep 17 00:00:00 2001
> From: Feng Xue <fxue@os.amperecomputing.com>
> Date: Tue, 21 Jan 2020 20:53:38 +0800
> Subject: [PATCH] Generalized value pass-through for self-recusive function
>
> ---
>  gcc/ipa-cp.c                       | 195 ++++++++++++++++++-----------
>  gcc/testsuite/g++.dg/ipa/pr93203.C |  95 ++++++++++++++
>  gcc/testsuite/gcc.dg/ipa/ipcp-1.c  |   2 +-
>  3 files changed, 216 insertions(+), 76 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr93203.C
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 17da1d8e8a7..64d23a34292 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c

...

> @@ -4817,19 +4867,12 @@ intersect_with_plats (class ipcp_param_lattices *plats,
>  	    break;
>  	  if (aglat->offset - offset == item->offset)
>  	    {
> -	      gcc_checking_assert (item->value);

I've been staring at this for quite a while, trying to figure out how
your patch can put NULL here before I realized it was just a clean-up
:-)  Sending such changes independently or pointing them out in the
email/ChangeLog makes review easier.

>  	      if (aglat->is_single_const ())
>  		{
>  		  tree value = aglat->values->value;
>  
>  		  if (values_equal_for_ipcp_p (item->value, value))
>  		    found = true;
> -		  else if (item->value == error_mark_node)
> -		    {
> -		      /* Replace unknown place holder value with real one.  */
> -		      item->value = value;
> -		      found = true;
> -		    }
>  		}
>  	      break;
>  	    }

...

> @@ -5564,7 +5610,6 @@ decide_whether_version_node (struct cgraph_node *node)
>  	}
>        clone = create_specialized_node (node, known_csts, known_contexts,
>  				       aggvals, callers);
> -      info = IPA_NODE_REF (node);

please either drop this change or change it to:

   gcc_checking_assert (info == IPA_NODE_REF (node));

this line of code was actually necessary when adding nodes possibly
invalidated addresses of all summaries - like fast_function_summary
classes still do.  So if we ever decide to use fast summaries we need a
test to remind us that info address must be obtained again.

Thanks for working on this!

Martin