From patchwork Wed Nov 6 22:24:40 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 289040 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)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id ACE3F2C014D for ; Thu, 7 Nov 2013 09:36:45 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=BM0dnGlYNS4hGfG48 0MB/2TkHX76qjvpxpaQSsCVE2CdtkkvFfROQFf6qO24b3Cf6y4rzYimp3l3GYF6G TsMFWPTSJhhwUGe/q/H6bk3l6kGyDjqfR+a4uLETmsir6+tNvL94mgXtBHmhQC+E eyWQ1xPkDGyX58MIQfCRMfyJb8= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=WrDV59gNm5f493UecYrMujD jPC4=; b=IHGFWib+Gu2ON3B2fY0uN9S4yEeEYZ/EAyTtUCeVivgQTiZt6Ml/gTv 0WGySZ9Jz2uhb5JPGS23Ov2lRkQ2YFD7gFUifKwJ3/UWfTr1+0U6Bj2hyjUyDXkx 75jXF8qTnLqQxdtmi3eaVkV8yVFgbnrG6260zxQsLaHI//D8ryfs= Received: (qmail 6999 invoked by alias); 6 Nov 2013 22:24:52 -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 6984 invoked by uid 89); 6 Nov 2013 22:24:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=AWL, BAYES_50, RDNS_NONE, SPF_HELO_PASS, SPF_PASS, URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Nov 2013 22:24:49 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA6MOgcL031963 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 6 Nov 2013 17:24:42 -0500 Received: from reynosa.quesejoda.com (vpn-52-207.rdu2.redhat.com [10.10.52.207]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id rA6MOen4003404; Wed, 6 Nov 2013 17:24:41 -0500 Message-ID: <527AC1A8.70904@redhat.com> Date: Wed, 06 Nov 2013 15:24:40 -0700 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Jakub Jelinek CC: gcc-patches Subject: [gomp4] rewrite simd clone argument adjustment to avoid regimplification References: <5277BAF0.6050506@redhat.com> <20131104154435.GS27813@tucnak.zalov.cz> In-Reply-To: <20131104154435.GS27813@tucnak.zalov.cz> On 11/04/13 08:44, Jakub Jelinek wrote: > I guess short time yes, but I wonder if it wouldn't be better to use > walk_gimple_op and do all changes in the callback. Instead of passing > adjustments pass around a struct containing the adjustments, current stmt > and the modified flag. You can use the val_only and is_lhs to determine > what you need to do (probably need to reset those two for the subtrees > to val_only = true, is_lhs = false and not walk subtrees of types) and you > could (if not val_only) immediately gimplify it properly (insert temporary > SSA_NAME setter before resp. store after depending on is_lhs). > Then you could avoid the regimplification. With the attached patch, we get rid of the ad-hoc argument adjustment system that is in place, and avoid regimplification altogether. I am using walk_gimple_op as suggested, thus cleaning up most of ipa_simd_modify_function_body. I have checked the following patch with the attached testcases that were previously ICEing, and with a handful of handcrafted tests that I checked manually (array references on lhs and rhs, vectors of pointers, etc). OK for branch? gcc/ChangeLog.gomp * ipa-prop.h (sra_ipa_get_adjustment_candidate): Protoize. * omp-low.c (struct modify_stmt_info): New. (ipa_simd_modify_function_body_ops_1): Remove. (ipa_simd_modify_stmt_ops): New. (ipa_simd_modify_function_body_ops): Remove. (ipa_simd_modify_function_body): Set new callback info. Remove special casing. Handle all operators with walk_gimple_op. * tree-sra.c (get_ssa_base_param): Add new argument. Use it. (disqualify_base_of_expr): Pass new argument to get_ssa_base_param. (sra_ipa_modify_expr): Abstract candidate search into... (sra_ipa_get_adjustment_candidate): ...here. diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 06296d1..6aebf8d 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -719,5 +719,8 @@ tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, tree, gimple_stmt_iterator *, bool); bool ipa_sra_modify_function_body (ipa_parm_adjustment_vec); bool sra_ipa_modify_expr (tree *, bool, ipa_parm_adjustment_vec); +ipa_parm_adjustment *sra_ipa_get_adjustment_candidate (tree *&, bool *, + ipa_parm_adjustment_vec, + bool); #endif /* IPA_PROP_H */ diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 94058af..0dd0676 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -11049,33 +11049,81 @@ simd_clone_init_simd_arrays (struct cgraph_node *node, return seq; } +/* Callback info for ipa_simd_modify_stmt_ops below. */ + +struct modify_stmt_info { + ipa_parm_adjustment_vec adjustments; + gimple stmt; + /* True if the parent statement was modified by + ipa_simd_modify_stmt_ops. */ + bool modified; +}; + +/* Callback for walk_gimple_op. + + Adjust operands from a given statement as specified in the + adjustments vector in the callback data. */ + static tree -ipa_simd_modify_function_body_ops_1 (tree *tp, int *walk_subtrees, void *data) +ipa_simd_modify_stmt_ops (tree *tp, + int *walk_subtrees ATTRIBUTE_UNUSED, + void *data) { struct walk_stmt_info *wi = (struct walk_stmt_info *) data; - ipa_parm_adjustment_vec *adjustments = (ipa_parm_adjustment_vec *) wi->info; + if (!SSA_VAR_P (*tp)) + { + /* Make sure we treat subtrees as a RHS. This makes sure that + when examining the `*foo' in *foo=x, the `foo' get treated as + a use properly. */ + wi->is_lhs = false; + wi->val_only = true; + return NULL_TREE; + } + struct modify_stmt_info *info = (struct modify_stmt_info *) wi->info; + struct ipa_parm_adjustment *cand + = sra_ipa_get_adjustment_candidate (tp, NULL, info->adjustments, true); + if (!cand) + return NULL_TREE; + tree t = *tp; + tree repl = make_ssa_name (create_tmp_var (TREE_TYPE (t), NULL), NULL); - if (DECL_P (t) || TREE_CODE (t) == SSA_NAME) - return (tree) sra_ipa_modify_expr (tp, true, *adjustments); + gimple stmt; + gimple_stmt_iterator gsi = gsi_for_stmt (info->stmt); + if (wi->is_lhs) + { + stmt = gimple_build_assign (unshare_expr (cand->reduction), repl); + gsi_insert_after (&gsi, stmt, GSI_SAME_STMT); + SSA_NAME_DEF_STMT (repl) = info->stmt; + } else - *walk_subtrees = 1; - return NULL_TREE; -} + { + /* You'd think we could skip the extra SSA variable when + wi->val_only=true, but we may have `*var' which will get + replaced into `*var_array[iter]' and will likely be something + not gimple. */ + stmt = gimple_build_assign (repl, unshare_expr (cand->reduction)); + gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); + } -/* Helper function for ipa_simd_modify_function_body. Make any - necessary adjustments for tree operators. */ + if (!useless_type_conversion_p (TREE_TYPE (*tp), TREE_TYPE (repl))) + { + tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*tp), repl); + *tp = vce; + } + else + *tp = repl; -static bool -ipa_simd_modify_function_body_ops (tree *tp, - ipa_parm_adjustment_vec *adjustments) -{ - struct walk_stmt_info wi; - memset (&wi, 0, sizeof (wi)); - wi.info = adjustments; - bool res = (bool) walk_tree (tp, ipa_simd_modify_function_body_ops_1, - &wi, NULL); - return res; + /* We have modified in place; update the SSA operands. */ + info->modified = false; + stmt = info->stmt; + update_stmt (stmt); + if (maybe_clean_eh_stmt (stmt)) + gimple_purge_dead_eh_edges (gimple_bb (stmt)); + + wi->is_lhs = false; + wi->val_only = true; + return NULL_TREE; } /* Traverse the function body and perform all modifications as @@ -11111,6 +11159,9 @@ ipa_simd_modify_function_body (struct cgraph_node *node, NULL_TREE, NULL_TREE); } + struct modify_stmt_info info; + info.adjustments = adjustments; + FOR_EACH_BB_FN (bb, DECL_STRUCT_FUNCTION (node->decl)) { gimple_stmt_iterator gsi; @@ -11119,9 +11170,13 @@ ipa_simd_modify_function_body (struct cgraph_node *node, while (!gsi_end_p (gsi)) { gimple stmt = gsi_stmt (gsi); + info.stmt = stmt; bool modified = false; - tree *t; - unsigned i; + struct walk_stmt_info wi; + + memset (&wi, 0, sizeof (wi)); + info.modified = false; + wi.info = &info; switch (gimple_code (stmt)) { @@ -11147,56 +11202,9 @@ ipa_simd_modify_function_body (struct cgraph_node *node, break; } - case GIMPLE_ASSIGN: - t = gimple_assign_lhs_ptr (stmt); - modified |= sra_ipa_modify_expr (t, false, adjustments); - - /* The LHS may have operands that also need adjusting - (e.g. `foo' in array[foo]). */ - modified |= ipa_simd_modify_function_body_ops (t, &adjustments); - - for (i = 0; i < gimple_num_ops (stmt); ++i) - { - t = gimple_op_ptr (stmt, i); - modified |= sra_ipa_modify_expr (t, false, adjustments); - } - break; - - case GIMPLE_CALL: - /* Operands must be processed before the lhs. */ - for (i = 0; i < gimple_call_num_args (stmt); i++) - { - t = gimple_call_arg_ptr (stmt, i); - modified |= sra_ipa_modify_expr (t, true, adjustments); - } - - if (gimple_call_lhs (stmt)) - { - t = gimple_call_lhs_ptr (stmt); - modified |= sra_ipa_modify_expr (t, false, adjustments); - } - break; - - case GIMPLE_ASM: - for (i = 0; i < gimple_asm_ninputs (stmt); i++) - { - t = &TREE_VALUE (gimple_asm_input_op (stmt, i)); - modified |= sra_ipa_modify_expr (t, true, adjustments); - } - for (i = 0; i < gimple_asm_noutputs (stmt); i++) - { - t = &TREE_VALUE (gimple_asm_output_op (stmt, i)); - modified |= sra_ipa_modify_expr (t, false, adjustments); - } - break; - default: - for (i = 0; i < gimple_num_ops (stmt); ++i) - { - t = gimple_op_ptr (stmt, i); - if (*t) - modified |= sra_ipa_modify_expr (t, true, adjustments); - } + walk_gimple_op (stmt, ipa_simd_modify_stmt_ops, &wi); + modified |= info.modified; break; } diff --git a/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c b/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c new file mode 100644 index 0000000..ef6fa11 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gomp/simd-clones-7.c @@ -0,0 +1,16 @@ +/* { dg-do compile { target i?86-*-* x86_64-*-* } } */ +/* { dg-options "-fopenmp -w" } */ + +int array[1000]; + +#pragma omp declare simd notinbranch simdlen(4) +void foo (int *a, int b) +{ + a[b] = 555; +} + +#pragma omp declare simd notinbranch simdlen(4) +void bar (int *a) +{ + *a = 555; +} diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 27938ab..36994f7 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -785,15 +785,17 @@ type_internals_preclude_sra_p (tree type, const char **msg) } } -/* If T is an SSA_NAME, return NULL if it is not a default def or return its - base variable if it is. Return T if it is not an SSA_NAME. */ +/* If T is an SSA_NAME, return NULL if it is not a default def or + return its base variable if it is. If IGNORE_DEFAULT_DEF is true, + the base variable is always returned, regardless if it is a default + def. Return T if it is not an SSA_NAME. */ static tree -get_ssa_base_param (tree t) +get_ssa_base_param (tree t, bool ignore_default_def) { if (TREE_CODE (t) == SSA_NAME) { - if (SSA_NAME_IS_DEFAULT_DEF (t)) + if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t)) return SSA_NAME_VAR (t); else return NULL_TREE; @@ -874,7 +876,7 @@ create_access (tree expr, gimple stmt, bool write) if (sra_mode == SRA_MODE_EARLY_IPA && TREE_CODE (base) == MEM_REF) { - base = get_ssa_base_param (TREE_OPERAND (base, 0)); + base = get_ssa_base_param (TREE_OPERAND (base, 0), false); if (!base) return NULL; ptr = true; @@ -1039,7 +1041,7 @@ disqualify_base_of_expr (tree t, const char *reason) t = get_base_address (t); if (sra_mode == SRA_MODE_EARLY_IPA && TREE_CODE (t) == MEM_REF) - t = get_ssa_base_param (TREE_OPERAND (t, 0)); + t = get_ssa_base_param (TREE_OPERAND (t, 0), false); if (t && DECL_P (t)) disqualify_candidate (t, reason); @@ -4485,35 +4487,35 @@ replace_removed_params_ssa_names (gimple stmt, return true; } -/* If the expression *EXPR should be replaced by a reduction of a parameter, do - so. ADJUSTMENTS is a pointer to a vector of adjustments. CONVERT - specifies whether the function should care about type incompatibility the - current and new expressions. If it is false, the function will leave - incompatibility issues to the caller. Return true iff the expression - was modified. */ +/* Given an expression, return an adjustment entry specifying the + transformation to be done on EXPR. If no suitable adjustment entry + was found, returns NULL. -bool -sra_ipa_modify_expr (tree *expr, bool convert, - ipa_parm_adjustment_vec adjustments) -{ - int i, len; - struct ipa_parm_adjustment *adj, *cand = NULL; - HOST_WIDE_INT offset, size, max_size; - tree base, src; + If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a + default def, otherwise bail on them. - len = adjustments.length (); + If CONVERT is non-NULL, this function will set *CONVERT if the + expression provided is a component reference that must be converted + upon return. ADJUSTMENTS is the adjustments vector. */ +ipa_parm_adjustment * +sra_ipa_get_adjustment_candidate (tree *&expr, bool *convert, + ipa_parm_adjustment_vec adjustments, + bool ignore_default_def) +{ if (TREE_CODE (*expr) == BIT_FIELD_REF || TREE_CODE (*expr) == IMAGPART_EXPR || TREE_CODE (*expr) == REALPART_EXPR) { expr = &TREE_OPERAND (*expr, 0); - convert = true; + if (convert) + *convert = true; } - base = get_ref_base_and_extent (*expr, &offset, &size, &max_size); + HOST_WIDE_INT offset, size, max_size; + tree base = get_ref_base_and_extent (*expr, &offset, &size, &max_size); if (!base || size == -1 || max_size == -1) - return false; + return NULL; if (TREE_CODE (base) == MEM_REF) { @@ -4521,13 +4523,15 @@ sra_ipa_modify_expr (tree *expr, bool convert, base = TREE_OPERAND (base, 0); } - base = get_ssa_base_param (base); + base = get_ssa_base_param (base, ignore_default_def); if (!base || TREE_CODE (base) != PARM_DECL) - return false; + return NULL; - for (i = 0; i < len; i++) + struct ipa_parm_adjustment *cand = NULL; + unsigned int len = adjustments.length (); + for (unsigned i = 0; i < len; i++) { - adj = &adjustments[i]; + struct ipa_parm_adjustment *adj = &adjustments[i]; if (adj->base == base && (adj->offset == offset || adj->remove_param)) @@ -4536,9 +4540,29 @@ sra_ipa_modify_expr (tree *expr, bool convert, break; } } + if (!cand || cand->copy_param || cand->remove_param) + return NULL; + return cand; +} + +/* If the expression *EXPR should be replaced by a reduction of a parameter, do + so. ADJUSTMENTS is a pointer to a vector of adjustments. CONVERT + specifies whether the function should care about type incompatibility the + current and new expressions. If it is false, the function will leave + incompatibility issues to the caller. Return true iff the expression + was modified. */ + +bool +sra_ipa_modify_expr (tree *expr, bool convert, + ipa_parm_adjustment_vec adjustments) +{ + struct ipa_parm_adjustment *cand + = sra_ipa_get_adjustment_candidate (expr, &convert, adjustments, false); + if (!cand) return false; + tree src; if (cand->by_ref) src = build_simple_mem_ref (cand->reduction); else