From patchwork Fri Jan 20 22:27:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 717947 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 3v4wLr6gfkz9ryr for ; Sat, 21 Jan 2017 09:27:29 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="fESj69sw"; 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:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=rBjV88hhM3OSn/m7r y084kNY+LNQXXqAfBFuVOa2JDWlk1uAqpVkTlgpJrLXOAgdii6U3AFbM5cQmiDcF q3+0syuN/1+UJX9df5yNTdBxeFiUG9jJezHNRHRtWwLGxyHWL261jMBjpEkZR6rW FybgU0tbtUm74VMrLLLBfURNKw= 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:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=MDG3NMdgIl4hLwJoKHKkkeD 1xFk=; b=fESj69swjteAhgk/Y4EsRkIYs12UKiHqLne+gx9n7do1IrCTjSZonRu rAeVDllPrkWEHewHwe+yitsZSBeIwqyvjrW34sBjUas4Vw1yHgwEAs0fU68ZuB0a 04FnRwboclngU/1ALvVKsSJmyvXN7rzEkVwz8f9afkUFT31cEn7w= Received: (qmail 30898 invoked by alias); 20 Jan 2017 22:27:21 -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 30804 invoked by uid 89); 20 Jan 2017 22:27:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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, 20 Jan 2017 22:27:07 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D24ECAD97; Fri, 20 Jan 2017 22:27:04 +0000 (UTC) Date: Fri, 20 Jan 2017 23:27:04 +0100 From: Martin Jambor To: Richard Biener Cc: gcc-patches@gcc.gnu.org, kugan Subject: Re: [PATCH] Fix PR78515 Message-ID: <20170120222704.4hbkmvjthvwkdp5y@virgil.suse.cz> Mail-Followup-To: Richard Biener , gcc-patches@gcc.gnu.org, kugan References: <20161125131935.ud4wpxebrrjo22yn@virgil.suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2 (2016-07-01) X-IsSubscribed: yes Hi, On Fri, Nov 25, 2016 at 02:55:29PM +0100, Richard Biener wrote: > On Fri, 25 Nov 2016, Martin Jambor wrote: > > ... > > > > There's still that odd 'stmt2' > > > hanging around that gets set to sth else than stmt with > > > > > > op1 = gimple_assign_rhs1 (stmt); > > > > > > if (TREE_CODE (op1) == SSA_NAME) > > > { > > > if (SSA_NAME_IS_DEFAULT_DEF (op1)) > > > index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1)); > > > else > > > { > > > index = load_from_param (fbi, info->descriptors, > > > SSA_NAME_DEF_STMT (op1)); > > > stmt2 = SSA_NAME_DEF_STMT (op1); > > > > > > I assume that the original code wanted to restrict its processing > > > to unary RHS of 'stmt' but still this "skips" arbitrary unary > > > operations in 'stmt'? But maybe I'm not understanding jump functions > > > here. If we have > > > > > > _2 = -param_1(D); > > > _3 = ~_2; > > > > > > and stmt is _3 then we create a unary pass through JF with - (and the ~ > > > gets lost?). > > > > > > > It definitely looks like that. Unary arithmetic jump functions have > > been added only recently with the IPA VRP propagation and I remember > > staring at the stmt2 thingy for a while during review but then > > apparently I forgot about it. > > > > It seems to me that the check should refer to stmt, I will do that and > > see what breaks and how the IL looks at that point and then decide > > where to go from there. > > it's the only use of stmt2 though, so it must have been added for some > reason... (maybe somebody wanted to handle simple copy-propagation?!). > I'd say rip it out and thus keep stmt2 == stmt. There must be > a testcase added for this... > So I have pondered about this some more and found out that while the current code really makes no sense, it is fortunately harmless because load_from_param will suceed only if it looks at a load from a PARM_DECL that does not have an SSA_NAME and so cannot have any arithmetic operation associated with it. That means that there cannot really be any difference between load_from_unmodified_param and load_from_param and so the patch below re-unifies them. It also removes the stmt2 variable from compute_complex_assign_jump_func which means that the function is actually more powerful now, able to do IPA-VRP on the added testcase... which kind of makes me wonder whether it is appropriate at this stage, but I'd prefer to put the code in order. Bootstrapped and tested on x86_64-linux. LTO-bootstrapped and testing of the result still running on the same architecture. OK for trunk if it succeeds? (The patch is intended to go on top of my fix for PR 79108). Sorry for not spotting this when reviewing the patch that introduced it in the first place, Martin 2017-01-20 Martin Jambor * ipa-prop.c (load_from_param_1): Removed. (load_from_unmodified_param): Bits from load_from_param_1 put back here. (load_from_param): Removed. (compute_complex_assign_jump_func): Removed stmt2 and just replaced it with stmt. Reverted back to use of load_from_unmodified_param. testsuite/ * gcc.dg/ipa/vrp8.c: New test. --- gcc/ipa-prop.c | 68 ++++++++++------------------------------- gcc/testsuite/gcc.dg/ipa/vrp8.c | 42 +++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 52 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/vrp8.c diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 4d77c9b25ef..512bcbed0cb 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -862,31 +862,6 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info *fbi, int index, return !modified; } -/* Main worker for load_from_unmodified_param and load_from_param. - If STMT is an assignment that loads a value from an parameter declaration, - return the index of the parameter in ipa_node_params. Otherwise return -1. */ - -static int -load_from_param_1 (struct ipa_func_body_info *fbi, - vec *descriptors, - gimple *stmt) -{ - int index; - tree op1; - - gcc_checking_assert (is_gimple_assign (stmt)); - op1 = gimple_assign_rhs1 (stmt); - if (TREE_CODE (op1) != PARM_DECL) - return -1; - - index = ipa_get_param_decl_index_1 (descriptors, op1); - if (index < 0 - || !parm_preserved_before_stmt_p (fbi, index, stmt, op1)) - return -1; - - return index; -} - /* If STMT is an assignment that loads a value from an parameter declaration, return the index of the parameter in ipa_node_params which has not been modified. Otherwise return -1. */ @@ -896,29 +871,22 @@ load_from_unmodified_param (struct ipa_func_body_info *fbi, vec *descriptors, gimple *stmt) { + int index; + tree op1; + if (!gimple_assign_single_p (stmt)) return -1; - return load_from_param_1 (fbi, descriptors, stmt); -} - -/* If STMT is an assignment that loads a value from an parameter declaration, - return the index of the parameter in ipa_node_params. Otherwise return -1. */ - -static int -load_from_param (struct ipa_func_body_info *fbi, - vec *descriptors, - gimple *stmt) -{ - if (!is_gimple_assign (stmt)) + op1 = gimple_assign_rhs1 (stmt); + if (TREE_CODE (op1) != PARM_DECL) return -1; - enum tree_code rhs_code = gimple_assign_rhs_code (stmt); - if ((get_gimple_rhs_class (rhs_code) != GIMPLE_SINGLE_RHS) - && (get_gimple_rhs_class (rhs_code) != GIMPLE_UNARY_RHS)) + index = ipa_get_param_decl_index_1 (descriptors, op1); + if (index < 0 + || !parm_preserved_before_stmt_p (fbi, index, stmt, op1)) return -1; - return load_from_param_1 (fbi, descriptors, stmt); + return index; } /* Return true if memory reference REF (which must be a load through parameter @@ -1154,7 +1122,6 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi, tree op1, tc_ssa, base, ssa; bool reverse; int index; - gimple *stmt2 = stmt; op1 = gimple_assign_rhs1 (stmt); @@ -1163,16 +1130,13 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi, if (SSA_NAME_IS_DEFAULT_DEF (op1)) index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1)); else - { - index = load_from_param (fbi, info->descriptors, - SSA_NAME_DEF_STMT (op1)); - stmt2 = SSA_NAME_DEF_STMT (op1); - } + index = load_from_unmodified_param (fbi, info->descriptors, + SSA_NAME_DEF_STMT (op1)); tc_ssa = op1; } else { - index = load_from_param (fbi, info->descriptors, stmt); + index = load_from_unmodified_param (fbi, info->descriptors, stmt); tc_ssa = gimple_assign_lhs (stmt); } @@ -1202,11 +1166,11 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi, break; } case GIMPLE_UNARY_RHS: - if (is_gimple_assign (stmt2) - && gimple_assign_rhs_class (stmt2) == GIMPLE_UNARY_RHS - && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt2))) + if (is_gimple_assign (stmt) + && gimple_assign_rhs_class (stmt) == GIMPLE_UNARY_RHS + && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt))) ipa_set_jf_unary_pass_through (jfunc, index, - gimple_assign_rhs_code (stmt2)); + gimple_assign_rhs_code (stmt)); default:; } return; diff --git a/gcc/testsuite/gcc.dg/ipa/vrp8.c b/gcc/testsuite/gcc.dg/ipa/vrp8.c new file mode 100644 index 00000000000..55832b0886e --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/vrp8.c @@ -0,0 +1,42 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-cp-details" } */ + +volatile int cond; +int abs (int); + +volatile int g; + +int __attribute__((noinline, noclone)) +take_address (int *p) +{ + g = *p; +} + +static int __attribute__((noinline, noclone)) +foo (int i) +{ + if (i < 5) + __builtin_abort (); + return 0; +} + +static int __attribute__((noinline, noclone)) +bar (int j) +{ + foo (~j); + foo (abs (j)); + foo (j); + take_address (&j); + return 0; +} + +int +main () +{ + for (unsigned int i = 0; i < 10; ++i) + bar (i); + + return 0; +} + +/* { dg-final { scan-ipa-dump-times "Setting value range of param 0 \\\[-10, 9\\\]" 1 "cp" } } */