On 11/06/13 15:48, Jakub Jelinek wrote:
On Wed, Nov 06, 2013 at 03:24:40PM -0700, Aldy Hernandez wrote:
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).
I'd like Martin to eyeball the ipa changes eventually.
Indeed! Likewise for all my previous changes to ipa-prop.[ch] and
tree-sra.c.
+ 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. */
I think it wouldn't hurt to e.g. do
if (TYPE_P (*tp))
*walk_subtrees = 0;
here (and drop ATTRIBUTE_UNUSED above.
Done.
+ 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);
Just do
tree repl = make_ssa_name (TREE_TYPE (t), NULL);
no need to create underlying vars since 4.8.
Done.
+ /* We have modified in place; update the SSA operands. */
+ info->modified = false;
So you always set modified to false? I was expecting you'd set
it to true here and defer update_stmt and maybe_clean_eh_stmt etc.
to after walk_gimple_op (so, do it only when all the changes on the stmt
have been performed). Plus, when you modify something, there is no need
to walk subtrees, so you can just do *walk_subtrees = 0; too.
Hmmm, good point. I've moved update_stmt and company to the caller, and
modified the caller to call regimplify_operands only for GIMPLE_RETURN
which is the special case.
Let me know if this is still ok so I can commit.
Thanks.
Aldy
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 9d0ddcd..14b8571 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11049,33 +11049,75 @@ 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, 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;
+ if (TYPE_P (*tp))
+ *walk_subtrees = 0;
+ 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 (TREE_TYPE (t), 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;
+ info->modified = true;
+ wi->is_lhs = false;
+ wi->val_only = true;
+ return NULL_TREE;
}
/* Traverse the function body and perform all modifications as
@@ -11111,6 +11153,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 +11164,12 @@ ipa_simd_modify_function_body (struct cgraph_node
*node,
while (!gsi_end_p (gsi))
{
gimple stmt = gsi_stmt (gsi);
- bool modified = false;
- tree *t;
- unsigned i;
+ info.stmt = stmt;
+ struct walk_stmt_info wi;
+
+ memset (&wi, 0, sizeof (wi));
+ info.modified = false;
+ wi.info = &info;
switch (gimple_code (stmt))
{
@@ -11137,7 +11185,8 @@ ipa_simd_modify_function_body (struct cgraph_node *node,
NULL, NULL),
old_retval);
gsi_replace (&gsi, stmt, true);
- modified = true;
+ gimple_regimplify_operands (stmt, &gsi);
+ info.modified = true;
}
else
{
@@ -11147,62 +11196,13 @@ 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);
break;
}
- if (modified)
+ if (info.modified)
{
- gimple_regimplify_operands (stmt, &gsi);
update_stmt (stmt);
if (maybe_clean_eh_stmt (stmt))
gimple_purge_dead_eh_edges (gimple_bb (stmt));
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
