From patchwork Fri Nov 24 16:34:17 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 841132 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-467873-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="PNqnq/8o"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3yk1xS6n9Wz9s4s for ; Sat, 25 Nov 2017 03:34:35 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:mime-version:content-type; q=dns; s=default; b=wiYmc/leD5yp1jZ7+3fVKMCWVWF/EImXiZJczJ5JfFmyGMQDNh ZbhEX8kuCXBwpCbHe0bs4PNIlTO46t0xSw7GE7pOJAf7r/Rd+vtkq8NzSKuuJ3ZI 9++mlcfCyxh2i9O7cfWECmqGDlwUR+lGnwRZDpwGiDlbYwXa2NdfIOMvg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:mime-version:content-type; s= default; bh=8vCOCWbjNUP6qua/YvEjITkxMRY=; b=PNqnq/8oGI5Bjw66ui4P rXLbVTxmc9XYqk/AfLSSY+W7tBS4fZD8eaxjA/tdlyls+XyM3zA/2XXthwC8L1TB 1u29YbTijQh1kT3PbvoTltAJUEQLI3SW7HMnrAd6TlmWPevWRkdQb/PUS1i9ZpPB GNfWZArucKEgdDeIk1Ji0hw= Received: (qmail 42931 invoked by alias); 24 Nov 2017 16:34:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 41193 invoked by uid 89); 24 Nov 2017 16:34:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KB_WAM_FROM_NAME_SINGLEWORD, SPF_PASS autolearn=ham version=3.3.2 spammy=type_size, TYPE_SIZE X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Nov 2017 16:34:21 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5220CAAAD; Fri, 24 Nov 2017 16:34:18 +0000 (UTC) From: Martin Jambor To: GCC Patches Cc: Jan Hubicka Subject: [PR 81248] Fix ipa-sra size check User-Agent: Notmuch/0.25.1 (https://notmuchmail.org) Emacs/25.3.1 (x86_64-suse-linux-gnu) Date: Fri, 24 Nov 2017 17:34:17 +0100 Message-ID: MIME-Version: 1.0 X-IsSubscribed: yes 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 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 + +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 +void f(const T& x) __attribute__((noinline)); +template +void f(const T& x) { + if constexpr(std::is_same, 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_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