From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126803 invoked by alias); 24 Nov 2017 17:21:23 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 126780 invoked by uid 89); 24 Nov 2017 17:21:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f175.google.com Received: from mail-wr0-f175.google.com (HELO mail-wr0-f175.google.com) (209.85.128.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Nov 2017 17:21:19 +0000 Received: by mail-wr0-f175.google.com with SMTP id s41so15518417wrc.7 for ; Fri, 24 Nov 2017 09:21:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:user-agent:in-reply-to:references :mime-version:content-transfer-encoding:subject:to:cc:from :message-id; bh=1iiPgXrjrcUJLOKSDFxt7YEgiRmpc4nbBc1vx8xOg0s=; b=KUThqhVaupYjb39xgt4ud1F+ab3JDN7SBAQfFlsMJY88unACPw6MxYIX9AkX8rPR3b cAJm2q8eTpIKQ1kMk3gogk6MKnOqGEENDKvLeSvvd5ZWMVwuNvr9si3k9OwouZq7jN0s WlwH44CQpxFUIA8tKs89iuzu+wxkms4kQzHUbRMH8X6AhvLMTHMm/oGxFzXWKgpN8frZ 95UPzTBXUjGuHgNz+iAHiQlwmj4NT/5160Yoi+ovouiokiUlRrLDqJc5EnO/k0N7ZhRm DP1EOB/XIrAhfw4Jrko4+nMSDwxTae2sBUotDkysleljukS1Xp0h/7LX+yV/vTAJyMPa /CKQ== X-Gm-Message-State: AJaThX4noNMoO/zVf3KfuELcZ2/cJjQnQB1ZIRtVWzYbJD2HH335izjm fnAgfZQ1VS8NV8HPwRry7dKREfXN X-Google-Smtp-Source: AGs4zMbmkArB1repVBzJnlXOORc1z/Q5wqj8J+1+j3Xf4Omxo/2y2nhkm//heJukkUsIfZBVZi1zMQ== X-Received: by 10.223.155.133 with SMTP id d5mr5555142wrc.132.1511544076321; Fri, 24 Nov 2017 09:21:16 -0800 (PST) Received: from android-97b5c0ce9bfced28.fritz.box (p5DCEC1E9.dip0.t-ipconnect.de. [93.206.193.233]) by smtp.gmail.com with ESMTPSA id 2sm6832698wmk.28.2017.11.24.09.21.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 24 Nov 2017 09:21:15 -0800 (PST) Date: Fri, 24 Nov 2017 18:05:00 -0000 User-Agent: K-9 Mail for Android In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PR 81248] Fix ipa-sra size check To: gcc-patches@gcc.gnu.org,Martin Jambor ,GCC Patches CC: Jan Hubicka From: Richard Biener Message-ID: X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg02219.txt.bz2 On November 24, 2017 5:34:17 PM GMT+01:00, Martin Jambor = 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.=20 Richard.=20 >Thanks, > >Martin > > >2017-11-23 Martin Jambor > > 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 =3D 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, >=20 > 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) >=20 > 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=3Dc++17 -fdump-tree-eipa_sra" } >+ >+ >+#include >+ >+typedef unsigned char __uint8_t; >+typedef __uint8_t uint8_t; >+ >+ >+struct A { >+ A() =3D default; >+ A(const A& o) =3D default; >+ A(const volatile A& o) : m1(o.m1) {} >+ uint8_t m1{0}; >+}; >+ >+volatile uint8_t v; >+ >+template >+void f(const T& x) __attribute__((noinline)); >+template >+void f(const T& x) { >+ if constexpr(std::is_same, A>::value) { >+ v =3D x.m1; >+ } >+ else { >+ v =3D 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" } */ >=20 > 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" } */ >=20 > 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 =3D 0; >+ int total_size =3D 0; > struct access *access, *res, **prev_acc_ptr =3D &res; > vec *access_vec; >=20 >@@ -4520,13 +4520,6 @@ splice_param_accesses (tree parm, bool *ro_grp) > i =3D j; > } >=20 >- if (POINTER_TYPE_P (TREE_TYPE (parm))) >- agg_size =3D tree_to_uhwi (TYPE_SIZE (TREE_TYPE (TREE_TYPE >(parm)))); >- else >- agg_size =3D tree_to_uhwi (TYPE_SIZE (TREE_TYPE (parm))); >- if (total_size >=3D 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; >=20 >@@ -4546,15 +4539,9 @@ decide_one_param_reduction (struct access *repr) > gcc_assert (cur_parm_size > 0); >=20 > if (POINTER_TYPE_P (TREE_TYPE (parm))) >- { >- by_ref =3D true; >- agg_size =3D tree_to_uhwi (TYPE_SIZE (TREE_TYPE (TREE_TYPE >(parm)))); >- } >+ by_ref =3D true; > else >- { >- by_ref =3D false; >- agg_size =3D cur_parm_size; >- } >+ by_ref =3D false; >=20 > if (dump_file) > { >@@ -4567,7 +4554,7 @@ decide_one_param_reduction (struct access *repr) > } >=20 > total_size =3D 0; >- new_param_count =3D 0; >+ int new_param_count =3D 0; >=20 > for (; repr; repr =3D repr->next_grp) > { >@@ -4595,22 +4582,28 @@ decide_one_param_reduction (struct access >*repr) >=20 > gcc_assert (new_param_count > 0); >=20 >- if (optimize_function_for_size_p (cfun)) >- parm_size_limit =3D cur_parm_size; >- else >- parm_size_limit =3D (PARAM_VALUE (PARAM_IPA_SRA_PTR_GROWTH_FACTOR) >- * cur_parm_size); >- >- if (total_size < agg_size >- && total_size <=3D 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 >=3D cur_parm_size) >+ return 0; > } > else >- return 0; >+ { >+ int parm_num_limit; >+ if (optimize_function_for_size_p (cfun)) >+ parm_num_limit =3D 1; >+ else >+ parm_num_limit =3D 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; > } >=20 >/* The order of the following enums is important, we need to do extra >work for