On November 24, 2017 5:34:17 PM GMT+01:00, Martin Jambor <mjam...@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 <mjam...@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
