* [PR 81248] Fix ipa-sra size check
@ 2017-11-24 17:09 Martin Jambor
2017-11-24 18:05 ` Richard Biener
0 siblings, 1 reply; 2+ messages in thread
From: Martin Jambor @ 2017-11-24 17:09 UTC (permalink / raw)
To: GCC Patches; +Cc: Jan Hubicka
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
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
--
2.15.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PR 81248] Fix ipa-sra size check
2017-11-24 17:09 [PR 81248] Fix ipa-sra size check Martin Jambor
@ 2017-11-24 18:05 ` Richard Biener
0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2017-11-24 18:05 UTC (permalink / raw)
To: gcc-patches, Martin Jambor, GCC Patches; +Cc: Jan Hubicka
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-11-24 17:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-24 17:09 [PR 81248] Fix ipa-sra size check Martin Jambor
2017-11-24 18:05 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).