From patchwork Wed Mar 29 15:32:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 744844 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 3vtWwM6SSZz9s2s for ; Thu, 30 Mar 2017 02:32:19 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="htxttuy5"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; q=dns; s=default; b=Eyaxz9YlAzmoeDLMoqEhx6VgUIf0s+ GRcglHUg1jwKRlHo5GmWo7aOoB6YFtOX5luSHaUhLyhCOf1AvMe/vF0HEDUsU5fC nNUe6gCH7fEY32o2pE2B2TXIHA4y5oyf9nfd86vOLCP5lHirmnWPoSzTxvTPfw8O lAZesBM3Uvwcs= 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:date :from:to:subject:message-id:references:mime-version:content-type :in-reply-to; s=default; bh=FmQG9NHSDVVhZRnUXe85RgarZmo=; b=htxt tuy5N+msQ2IhqCwItwcSRUeTuCqAB9epZrWJM73amLtYVTumad2J6LJlSxR2Z5uo eyNIOj8OoHu2c0VHETlzgtXXrc43z7DBCEZT1V3YtuX5ZMg8ta7BMFSEP2y/sqGb kkIWGPiiKEZjVAk6/j4o8rwY6N1aQxl0aY339zo= Received: (qmail 96474 invoked by alias); 29 Mar 2017 15:32:06 -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 96435 invoked by uid 89); 29 Mar 2017 15:32:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.2 spammy=wine, recheck, promised, H*Ad:U*jh 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; Wed, 29 Mar 2017 15:32:03 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id DCC39AD0D for ; Wed, 29 Mar 2017 15:32:01 +0000 (UTC) Date: Wed, 29 Mar 2017 17:32:01 +0200 From: Martin Jambor To: Richard Biener , GCC Patches , Jan Hubicka Subject: Re: [PR 77333] Fix fntypes of calls calling clones Message-ID: <20170329153201.gyixz2rngo6xftsk@virgil.suse.cz> Mail-Followup-To: Richard Biener , GCC Patches , Jan Hubicka References: <20170310150718.xeg374b2z3lgicon@virgil.suse.cz> <20170316165751.w4h5kuzdzpall3xp@virgil.suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170316165751.w4h5kuzdzpall3xp@virgil.suse.cz> User-Agent: Mutt/1.6.2 (2016-07-01) X-IsSubscribed: yes Hi, On Thu, Mar 16, 2017 at 05:57:51PM +0100, Martin Jambor wrote: > Hi, > > On Mon, Mar 13, 2017 at 01:46:47PM +0100, Richard Biener wrote: > > On Fri, 10 Mar 2017, Martin Jambor wrote: > > > > > Hi, > > > > > > PR 77333 is a i686-windows target bug, which however has its root in > > > our general mechanism of adjusting gimple statements when redirecting > > > call graph edge. Basically, these three things trigger it: > > > > > > 1) IPA-CP figures out that the this parameter of a C++ class method is > > > unused and because the class is in an anonymous namespace, it can > > > be removed and all calls adjusted. That effectively changes a > > > normal method into a static method and so internally, its type > > > changes from METHOD_TYPE to FUNCTION_TYPE. > > > > > > 2) Since the fix of PR 57330, we do not update gimple_call_fntype to > > > match the new type, in fact we explicitely set it to the old, now > > > invalid, type (see redirect_call_stmt_to_callee in cgraph.c). > > > > > > 3) Function ix86_get_callcvt which decides on call ABI, ends with the > > > following condition: > > > > > > if (ret != 0 > > > || is_stdarg > > > || TREE_CODE (type) != METHOD_TYPE > > > || ix86_function_type_abi (type) != MS_ABI) > > > return IX86_CALLCVT_CDECL | ret; > > > > > > return IX86_CALLCVT_THISCALL; > > > > > > ...and since now the callee is no longer a METHOD_TYPE but callers > > > still think that they are, leading to calling convention mismatches > > > and subsequent crashes. It took me quite a lot of time to come up > > > with a small testcase (reproducible using wine) but eventually I > > > managed. > > > > > > The fix is not to do 2) above, but doing so without re-introducing PR > > > 57330, of course. ... > > > > In general I am sympathetic with not doing any IPA propagation > > across call stmt signature incompatibilties. Of course we may > > be still too strict in those compatibility check... > > > > > So the alternative would be to re-check when doing the gimple > > > statement adjustment and if the types match, then set the correct new > > > gimple_fntype and if they don't... then we can either leave it be or > > > just run the same type transformation on it as we did on the callee, > > > though they would be bogus either way. That is implemented in the > > > attached patch. > > ... > After talking to Honza today, we decided to probably go this route and > use the patch doing the type conversion at acall-sites when necessary. > Honza promised to review the patch soon (he wants to figure out why > former_clone_of can be NULL, something I decided not to bother about > since at that time I thought the other approach was going to be > preferable). > and this is a slightly adjusted patch that is a result of what we talked about. I know that it is potentially disruptive change, so I have tested it with: - bootstrap and testing and LTO-bootstrap and testing on x86_64-linux, - bootstrap and testing on i686-linux, ppc64le-linux and ia64-linux - bootstrap on aarch64-linux (no testing because there is no dejagnu installed on gcc117.fsffrance.org), - testing on i686-w64-mingw32 on Linux+wine, and - testing on powerpc-aix is underway. OK for trunk (and subsequently to backport to gcc 6 and 5)? Thanks, Martin 2017-03-24 Martin Jambor PR ipa/77333 * cgraph.h (cgraph_build_function_type_skip_args): Declare. * cgraph.c (redirect_call_stmt_to_callee): Set gimple fntype so that it reflects the signature changes performed at the callee side. * cgraphclones.c (build_function_type_skip_args): Make public, renamed to cgraph_build_function_type_skip_args. (build_function_decl_skip_args): Adjust call to the above function. testsuite/ * g++.dg/ipa/pr77333.C: New test. --- gcc/cgraph.c | 17 +++++++++- gcc/cgraph.h | 2 ++ gcc/cgraphclones.c | 9 +++--- gcc/testsuite/g++.dg/ipa/pr77333.C | 65 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/ipa/pr77333.C diff --git a/gcc/cgraph.c b/gcc/cgraph.c index 839388496ee..92ae0910c60 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -1424,8 +1424,23 @@ cgraph_edge::redirect_call_stmt_to_callee (void) if (skip_bounds) new_stmt = chkp_copy_call_skip_bounds (new_stmt); + tree old_fntype = gimple_call_fntype (e->call_stmt); gimple_call_set_fndecl (new_stmt, e->callee->decl); - gimple_call_set_fntype (new_stmt, gimple_call_fntype (e->call_stmt)); + cgraph_node *origin = e->callee; + while (origin->clone_of) + origin = origin->clone_of; + + if ((origin->former_clone_of + && old_fntype == TREE_TYPE (origin->former_clone_of)) + || old_fntype == TREE_TYPE (origin->decl)) + gimple_call_set_fntype (new_stmt, TREE_TYPE (e->callee->decl)); + else + { + bitmap skip = e->callee->clone.combined_args_to_skip; + tree t = cgraph_build_function_type_skip_args (old_fntype, skip, + false); + gimple_call_set_fntype (new_stmt, t); + } if (gimple_vdef (new_stmt) && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME) diff --git a/gcc/cgraph.h b/gcc/cgraph.h index 3889a3e1701..62cebd9e55a 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -2326,6 +2326,8 @@ void tree_function_versioning (tree, tree, vec *, void dump_callgraph_transformation (const cgraph_node *original, const cgraph_node *clone, const char *suffix); +tree cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip, + bool skip_return); /* In cgraphbuild.c */ int compute_call_stmt_bb_frequency (tree, basic_block bb); diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c index c2337e84553..69572b926c4 100644 --- a/gcc/cgraphclones.c +++ b/gcc/cgraphclones.c @@ -152,9 +152,9 @@ cgraph_edge::clone (cgraph_node *n, gcall *call_stmt, unsigned stmt_uid, /* Build variant of function type ORIG_TYPE skipping ARGS_TO_SKIP and the return value if SKIP_RETURN is true. */ -static tree -build_function_type_skip_args (tree orig_type, bitmap args_to_skip, - bool skip_return) +tree +cgraph_build_function_type_skip_args (tree orig_type, bitmap args_to_skip, + bool skip_return) { tree new_type = NULL; tree args, new_args = NULL; @@ -219,7 +219,8 @@ build_function_decl_skip_args (tree orig_decl, bitmap args_to_skip, if (prototype_p (new_type) || (skip_return && !VOID_TYPE_P (TREE_TYPE (new_type)))) new_type - = build_function_type_skip_args (new_type, args_to_skip, skip_return); + = cgraph_build_function_type_skip_args (new_type, args_to_skip, + skip_return); TREE_TYPE (new_decl) = new_type; /* For declarations setting DECL_VINDEX (i.e. methods) diff --git a/gcc/testsuite/g++.dg/ipa/pr77333.C b/gcc/testsuite/g++.dg/ipa/pr77333.C new file mode 100644 index 00000000000..1ef997f7a54 --- /dev/null +++ b/gcc/testsuite/g++.dg/ipa/pr77333.C @@ -0,0 +1,65 @@ +// { dg-do run } +// { dg-options "-O2 -fno-ipa-sra" } + +volatile int global; +int __attribute__((noinline, noclone)) +get_data (int i) +{ + global = i; + return i; +} + +typedef int array[32]; + +namespace { + +char buf[512]; + +class A +{ +public: + int field; + char *s; + + A() : field(223344) + { + s = buf; + } + + int __attribute__((noinline)) + foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, + int k, int l, int m, int n, int o, int p, int q, int r, int s, int t) + { + global = a+b+c+d+e+f+g+h+i+j+k+l+m+n+o+p+q+r+s+t; + return global; + } + + int __attribute__((noinline)) + bar() + { + int r = foo (get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1), + get_data (1), get_data (1), get_data (1), get_data (1)); + + if (field != 223344) + __builtin_abort (); + return 0; + } +}; + +} + +int main (int argc, char **argv) +{ + A a; + int r = a.bar(); + r = a.bar (); + if (a.field != 223344) + __builtin_abort (); + if (global != 20) + __builtin_abort (); + + return r; +}