[PR,81248] Fix ipa-sra size check

Hi,

PR 81248 is a missed-optimization bug when SRA refuses to replace a
pointer to a one-field structure to just passing the field by value.
The problem is a bogus check which compares the size of the replacement
with the size of the aggregate, even when it is passed by reference,
which was not the intention.

The check was also performed in two places.  This patch moves it only to
one and changes it as it was intended.  In the process I changed the
meaning of PARAM_IPA_SRA_PTR_GROWTH_FACTOR a bit and now it also limits
the number of new parameters as well as their total size, so that (by
default) we never create more than two replacements for an aggregate
passed by reference (because without it I have seen replacements by four
32-bit floats, for example).

I had to disable IPA-SRA for two testcases.  The SSA-PRE is testing
hoisting of loads that are is now done by IPA-SRA.  ipcp-cstagg-2.c now
unfortunately exhibits PR 80735.  But the situation is worse than
without the patch only for structures containing nothing but a function
pointer which I hope is not an interesting case.  I am still in the
process of rewriting IPA-SRA to a real IPA pass, now hope to have it
ready in early stage 1, and that should fix this issue, among others.

The patch has passed bootstrap and testing on x86_64-linux, OK for
trunk?

Thanks,

Martin


2017-11-23  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/81248
	* tree-sra.c (splice_param_accesses): Remove size check.
	(decide_one_param_reduction): Fix size check.
	* gimple-pretty-print.c (dump_profile): Silence warning.
	* params.def (PARAM_IPA_SRA_PTR_GROWTH_FACTOR): Adjust description.

	testsuite/
	* g++.dg/ipa/pr81248.C: New test.
	* gcc.dg/tree-ssa/ssa-pre-31.c: Disable IPA-SRA.
	* gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c: Likewise.
---
 gcc/gimple-pretty-print.c                  |  2 +-
 gcc/params.def                             |  4 +--
 gcc/testsuite/g++.dg/ipa/pr81248.C         | 40 ++++++++++++++++++++++
 gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c   |  2 +-
 gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c |  2 +-
 gcc/tree-sra.c                             | 55 +++++++++++++-----------------
 6 files changed, 69 insertions(+), 36 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr81248.C

On November 24, 2017 5:34:17 PM GMT+01:00, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>PR 81248 is a missed-optimization bug when SRA refuses to replace a
>pointer to a one-field structure to just passing the field by value.
>The problem is a bogus check which compares the size of the replacement
>with the size of the aggregate, even when it is passed by reference,
>which was not the intention.
>
>The check was also performed in two places.  This patch moves it only
>to
>one and changes it as it was intended.  In the process I changed the
>meaning of PARAM_IPA_SRA_PTR_GROWTH_FACTOR a bit and now it also limits
>the number of new parameters as well as their total size, so that (by
>default) we never create more than two replacements for an aggregate
>passed by reference (because without it I have seen replacements by
>four
>32-bit floats, for example).
>
>I had to disable IPA-SRA for two testcases.  The SSA-PRE is testing
>hoisting of loads that are is now done by IPA-SRA.  ipcp-cstagg-2.c now
>unfortunately exhibits PR 80735.  But the situation is worse than
>without the patch only for structures containing nothing but a function
>pointer which I hope is not an interesting case.  I am still in the
>process of rewriting IPA-SRA to a real IPA pass, now hope to have it
>ready in early stage 1, and that should fix this issue, among others.
>
>The patch has passed bootstrap and testing on x86_64-linux, OK for
>trunk?

OK. 

Richard. 

>Thanks,
>
>Martin
>
>
>2017-11-23  Martin Jambor  <mjambor@suse.cz>
>
>	PR tree-optimization/81248
>	* tree-sra.c (splice_param_accesses): Remove size check.
>	(decide_one_param_reduction): Fix size check.
>	* gimple-pretty-print.c (dump_profile): Silence warning.
>	* params.def (PARAM_IPA_SRA_PTR_GROWTH_FACTOR): Adjust description.
>
>	testsuite/
>	* g++.dg/ipa/pr81248.C: New test.
>	* gcc.dg/tree-ssa/ssa-pre-31.c: Disable IPA-SRA.
>	* gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c: Likewise.
>---
> gcc/gimple-pretty-print.c                  |  2 +-
> gcc/params.def                             |  4 +--
> gcc/testsuite/g++.dg/ipa/pr81248.C         | 40 ++++++++++++++++++++++
> gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c   |  2 +-
> gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c |  2 +-
>gcc/tree-sra.c                             | 55
>+++++++++++++-----------------
> 6 files changed, 69 insertions(+), 36 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/ipa/pr81248.C
>
>diff --git a/gcc/gimple-pretty-print.c b/gcc/gimple-pretty-print.c
>index 55c623e37bb..8bcc4e31bfb 100644
>--- a/gcc/gimple-pretty-print.c
>+++ b/gcc/gimple-pretty-print.c
>@@ -84,7 +84,7 @@ debug_gimple_stmt (gimple *gs)
> static const char *
> dump_profile (profile_count &count)
> {
>-  char *buf;
>+  char *buf = NULL;
>   if (!count.initialized_p ())
>     return "";
>   if (count.ipa_p ())
>diff --git a/gcc/params.def b/gcc/params.def
>index 89915d4fc7f..93bd2cf75fe 100644
>--- a/gcc/params.def
>+++ b/gcc/params.def
>@@ -971,8 +971,8 @@ DEFPARAM (PARAM_MIN_NONDEBUG_INSN_UID,
> 
> DEFPARAM (PARAM_IPA_SRA_PTR_GROWTH_FACTOR,
> 	  "ipa-sra-ptr-growth-factor",
>-	  "Maximum allowed growth of size of new parameters ipa-sra replaces
>"
>-	  "a pointer to an aggregate with.",
>+	  "Maximum allowed growth of number and total size of new parameters
>"
>+	  "that ipa-sra replaces a pointer to an aggregate with.",
> 	  2, 0, 0)
> 
> DEFPARAM (PARAM_TM_MAX_AGGREGATE_SIZE,
>diff --git a/gcc/testsuite/g++.dg/ipa/pr81248.C
>b/gcc/testsuite/g++.dg/ipa/pr81248.C
>new file mode 100644
>index 00000000000..d55d2e751e8
>--- /dev/null
>+++ b/gcc/testsuite/g++.dg/ipa/pr81248.C
>@@ -0,0 +1,40 @@
>+//  { dg-do compile }
>+// { dg-options "-O2 -std=c++17 -fdump-tree-eipa_sra" }
>+
>+
>+#include <type_traits>
>+
>+typedef unsigned char __uint8_t;
>+typedef __uint8_t uint8_t;
>+
>+
>+struct A {
>+    A() = default;
>+    A(const A& o) = default;
>+    A(const volatile A& o) : m1(o.m1) {}
>+    uint8_t m1{0};
>+};
>+
>+volatile uint8_t v;
>+
>+template<typename T>
>+void f(const T& x) __attribute__((noinline));
>+template<typename T>
>+void f(const T& x) {
>+    if constexpr(std::is_same<std::remove_cv_t<T>, A>::value) {
>+        v = x.m1;
>+    }
>+    else {
>+        v = x;
>+    }
>+}
>+
>+uint8_t n1;
>+A n2;
>+
>+int main() {
>+    f(n1);
>+    f(n2);
>+}
>+
>+// { dg-final { scan-tree-dump-times "Adjusting call" 2 "eipa_sra" } }
>diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
>b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
>index f82014024d4..c1b6f0f73a3 100644
>--- a/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
>+++ b/gcc/testsuite/gcc.dg/ipa/ipcp-cstagg-2.c
>@@ -1,5 +1,5 @@
> /* { dg-do compile } */
>-/* { dg-options "-O3 -fdump-ipa-cp-details" } */
>+/* { dg-options "-O3 -fdump-ipa-cp-details -fno-ipa-sra" } */
> 
> typedef struct S
> {
>diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c
>b/gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c
>index 0dface557be..6a33b942ad5 100644
>--- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c
>+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-pre-31.c
>@@ -1,5 +1,5 @@
> /* { dg-do compile } */
>-/* { dg-options "-O2 -fdump-tree-pre" } */
>+/* { dg-options "-O2 -fdump-tree-pre -fno-ipa-sra" } */
> 
> typedef struct {
>     unsigned int key;
>diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>index db490b20c3e..866cff0edb0 100644
>--- a/gcc/tree-sra.c
>+++ b/gcc/tree-sra.c
>@@ -4453,7 +4453,7 @@ static struct access *
> splice_param_accesses (tree parm, bool *ro_grp)
> {
>   int i, j, access_count, group_count;
>-  int agg_size, total_size = 0;
>+  int total_size = 0;
>   struct access *access, *res, **prev_acc_ptr = &res;
>   vec<access_p> *access_vec;
> 
>@@ -4520,13 +4520,6 @@ splice_param_accesses (tree parm, bool *ro_grp)
>       i = j;
>     }
> 
>-  if (POINTER_TYPE_P (TREE_TYPE (parm)))
>-    agg_size = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (TREE_TYPE
>(parm))));
>-  else
>-    agg_size = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (parm)));
>-  if (total_size >= agg_size)
>-    return NULL;
>-
>   gcc_assert (group_count > 0);
>   return res;
> }
>@@ -4537,7 +4530,7 @@ splice_param_accesses (tree parm, bool *ro_grp)
> static int
> decide_one_param_reduction (struct access *repr)
> {
>-  int total_size, cur_parm_size, agg_size, new_param_count,
>parm_size_limit;
>+  HOST_WIDE_INT total_size, cur_parm_size;
>   bool by_ref;
>   tree parm;
> 
>@@ -4546,15 +4539,9 @@ decide_one_param_reduction (struct access *repr)
>   gcc_assert (cur_parm_size > 0);
> 
>   if (POINTER_TYPE_P (TREE_TYPE (parm)))
>-    {
>-      by_ref = true;
>-      agg_size = tree_to_uhwi (TYPE_SIZE (TREE_TYPE (TREE_TYPE
>(parm))));
>-    }
>+    by_ref = true;
>   else
>-    {
>-      by_ref = false;
>-      agg_size = cur_parm_size;
>-    }
>+    by_ref = false;
> 
>   if (dump_file)
>     {
>@@ -4567,7 +4554,7 @@ decide_one_param_reduction (struct access *repr)
>     }
> 
>   total_size = 0;
>-  new_param_count = 0;
>+  int new_param_count = 0;
> 
>   for (; repr; repr = repr->next_grp)
>     {
>@@ -4595,22 +4582,28 @@ decide_one_param_reduction (struct access
>*repr)
> 
>   gcc_assert (new_param_count > 0);
> 
>-  if (optimize_function_for_size_p (cfun))
>-    parm_size_limit = cur_parm_size;
>-  else
>-    parm_size_limit = (PARAM_VALUE (PARAM_IPA_SRA_PTR_GROWTH_FACTOR)
>-                       * cur_parm_size);
>-
>-  if (total_size < agg_size
>-      && total_size <= parm_size_limit)
>+  if (!by_ref)
>     {
>-      if (dump_file)
>-	fprintf (dump_file, "    ....will be split into %i components\n",
>-		 new_param_count);
>-      return new_param_count;
>+      if (total_size >= cur_parm_size)
>+	return 0;
>     }
>   else
>-    return 0;
>+    {
>+      int parm_num_limit;
>+      if (optimize_function_for_size_p (cfun))
>+	parm_num_limit = 1;
>+      else
>+	parm_num_limit = PARAM_VALUE (PARAM_IPA_SRA_PTR_GROWTH_FACTOR);
>+
>+      if (new_param_count > parm_num_limit
>+	  || total_size > (parm_num_limit * cur_parm_size))
>+	return 0;
>+    }
>+
>+  if (dump_file)
>+    fprintf (dump_file, "    ....will be split into %i components\n",
>+	     new_param_count);
>+  return new_param_count;
> }
> 
>/* The order of the following enums is important, we need to do extra
>work for