Re: [PATCH] ipa: Avoid duplicate replacements in IPA-SRA transformation phase

[Martin Jambor <mjambor@suse.cz>]

Hello,

and ping, please.  (In my copy I have fixed the formatting issue spotted
by Jakub.)

Martin

On Fri, Mar 15 2024, Martin Jambor wrote:
> Hi,
>
> when the analysis part of IPA-SRA figures out that it would split out
> a scalar part of an aggregate which is known by IPA-CP to contain a
> known constant, it skips it knowing that the transformation part looks
> at IPA-CP aggregate results too and does the right thing (which can
> include doing the propagation in GIMPLE because that is the last
> moment the parameter exists).
>
> However, when IPA-SRA wants to split out a smaller non-aggregate out
> of an aggregate, which happens to be of the same size as a known
> scalar constant at the same offset, the transformation bit fails to
> recognize the situation, tries to do both splitting and constant
> propagation and in PR 111571 testcase creates a nonsensical call
> statement on which the call redirection then ICEs.
>
> Fixed by making sure we don't try to do two replacements of the same
> part of the same parameter.
>
> The look-up among replacements requires these are sorted and this
> patch just sorts them if they are not already sorted before each new
> look-up.  The worst number of sortings that can happen is number of
> parameters which are both split and have aggregate constants times
> param_ipa_max_agg_items (default 16).  I don't think complicating the
> source code to optimize for this unlikely case is worth it but if need
> be, it can of course be done.
>
> Bootstrapped and tested on x86_64-linux.  OK for master and eventually
> also the gcc-13 branch?
>
> Thanks,
>
> Martin
>
>
>
> gcc/ChangeLog:
>
> 2024-03-15  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/111571
> 	* ipa-param-manipulation.cc
> 	(ipa_param_body_adjustments::common_initialization): Avoid creating
> 	duplicate replacement entries.
>
> gcc/testsuite/ChangeLog:
>
> 2024-03-15  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/111571
> 	* gcc.dg/ipa/pr111571.c: New test.
> ---
>  gcc/ipa-param-manipulation.cc       | 16 ++++++++++++++++
>  gcc/testsuite/gcc.dg/ipa/pr111571.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr111571.c
>
> diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc
> index 3e0df6a6f77..4c6337cc563 100644
> --- a/gcc/ipa-param-manipulation.cc
> +++ b/gcc/ipa-param-manipulation.cc
> @@ -1525,6 +1525,22 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
>  		     replacement with a constant (for split aggregates passed
>  		     by value).  */
>  
> +		  if (split[parm_num])
> +		    {
> +		      /* We must be careful not to add a duplicate
> +			 replacement. */
> +		      sort_replacements ();
> +		      ipa_param_body_replacement *pbr =
> +			lookup_replacement_1 (m_oparms[parm_num],
> +					      av.unit_offset);
> +		      if (pbr)
> +			{
> +			  /* Otherwise IPA-SRA should have bailed out.  */
> +			  gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (pbr->repl)));
> +			  continue;
> +			}
> +		    }
> +
>  		  tree repl;
>  		  if (av.by_ref)
>  		    repl = av.value;
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr111571.c b/gcc/testsuite/gcc.dg/ipa/pr111571.c
> new file mode 100644
> index 00000000000..2a4adc608db
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr111571.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2"  } */
> +
> +struct a {
> +  int b;
> +};
> +struct c {
> +  long d;
> +  struct a e;
> +  long f;
> +};
> +int g, h, i;
> +int j() {return 0;}
> +static void k(struct a l, int p) {
> +  if (h)
> +    g = 0;
> +  for (; g; g = j())
> +    if (l.b)
> +      break;
> +}
> +static void m(struct c l) {
> +  k(l.e, l.f);
> +  for (;; --i)
> +    ;
> +}
> +int main() {
> +  struct c n = {10, 9};
> +  m(n);
> +}
> -- 
> 2.44.0