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

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

[Martin Jambor]

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

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

[Jakub Jelinek]

On Fri, Mar 15, 2024 at 06:57:18PM +0100, Martin Jambor wrote:
> --- 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);

Formatting nit, the = should be on the next line before
lookup_replacement_1.
		      ipa_param_body_replacement *pbr
			= lookup_replacement_1 (m_oparms[parm_num],
						av.unit_offset);

	Jakub

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

[Martin Jambor]

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

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

[Jan Hubicka]

> > 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.
OK,
thanks!
Honza