Re: [PR 81248] Fix ipa-sra size check

[Richard Biener <richard.guenther@gmail.com>]

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