On Fri, 18 Apr 2014, Jakub Jelinek wrote:
> Hi!
>
> This patch fixes the adjustments performed by ipa_simd_modify_function_body
> on omp declare simd clones.
> Previously we've been trying to replace typically SSA_NAMEs with underlying
> PARM_DECLs of the to be replaced arguments with loads/stores from/to
> array refs (that will be hopefully vectorized) right around the referencing
> stmt, but:
> 1) this can't really work well if there is any life range overlap in SSA_NAMEs
> with the same underlying PARM_DECL
> 2) PHIs weren't handled at all (neither PHI arguments, nor lhs of the PHIs)
> 3) for addressable PARM_DECLs the code pretty much assumed the same thing
> can be done too
>
> This patch instead adjusts all SSA_NAMEs with SSA_NAME_VAR equal to the to
> be replaced PARM_DECLs to a new underlying VAR_DECL, only changes the
> (D) SSA_NAME to a load done at the start of the entry block, and for
> addressable PARM_DECLs adjusts them such that they don't have to be
> regimplified (as we replace say address of a PARM_DECL which is a
> gimple_min_invariant with array ref with variable index which is not
> gimple_min_invariant, we need to force the addresses into SSA_NAMEs).
>
> The tree-dfa.c fix is what I've discovered while writing the patch,
> if htab_find_slot_with_hash (..., NO_INSERT) fails to find something
> in the hash table (most likely not actually needed by the patch, discovered
> that just because the patch was buggy initially), it returns NULL rather
> than address of some slot which will contain NULL.
Probably doesn't matter in practice as we are clearing a default-def
only if it is one (and thus it is recorded).
> Bootstrapped/regtested on x86_64-linux and i686-linux.
>
> Richard, does this look reasonable?
Yeah.
Thanks,
Richard.
> 2014-04-18 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/60823
> * omp-low.c (ipa_simd_modify_function_body): Go through
> all SSA_NAMEs and for those refering to vector arguments
> which are going to be replaced adjust SSA_NAME_VAR and,
> if it is a default definition, change it into a non-default
> definition assigned at the beginning of function from new_decl.
> (ipa_simd_modify_stmt_ops): Rewritten.
> * tree-dfa.c (set_ssa_default_def): When removing default def,
> check for NULL loc instead of NULL *loc.
>
> * c-c++-common/gomp/pr60823-1.c: New test.
> * c-c++-common/gomp/pr60823-2.c: New test.
> * c-c++-common/gomp/pr60823-3.c: New test.
>
> --- gcc/omp-low.c.jj 2014-04-17 14:48:59.076025713 +0200
> +++ gcc/omp-low.c 2014-04-18 12:00:16.666701773 +0200
> @@ -11281,45 +11281,53 @@ static tree
> ipa_simd_modify_stmt_ops (tree *tp, int *walk_subtrees, void *data)
> {
> struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> - if (!SSA_VAR_P (*tp))
> + struct modify_stmt_info *info = (struct modify_stmt_info *) wi->info;
> + tree *orig_tp = tp;
> + if (TREE_CODE (*tp) == ADDR_EXPR)
> + tp = &TREE_OPERAND (*tp, 0);
> + struct ipa_parm_adjustment *cand = NULL;
> + if (TREE_CODE (*tp) == PARM_DECL)
> + cand = ipa_get_adjustment_candidate (&tp, NULL, info->adjustments, true);
> + else
> {
> - /* 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
> - = 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);
> -
> - gimple stmt;
> - gimple_stmt_iterator gsi = gsi_for_stmt (info->stmt);
> - if (wi->is_lhs)
> - {
> - stmt = gimple_build_assign (unshare_expr (cand->new_decl), repl);
> - gsi_insert_after (&gsi, stmt, GSI_SAME_STMT);
> - SSA_NAME_DEF_STMT (repl) = info->stmt;
> }
> +
> + tree repl = NULL_TREE;
> + if (cand)
> + repl = unshare_expr (cand->new_decl);
> else
> {
> - /* 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->new_decl));
> - gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
> + if (tp != orig_tp)
> + {
> + *walk_subtrees = 0;
> + bool modified = info->modified;
> + info->modified = false;
> + walk_tree (tp, ipa_simd_modify_stmt_ops, wi, wi->pset);
> + if (!info->modified)
> + {
> + info->modified = modified;
> + return NULL_TREE;
> + }
> + info->modified = modified;
> + repl = *tp;
> + }
> + else
> + return NULL_TREE;
> }
>
> - if (!useless_type_conversion_p (TREE_TYPE (*tp), TREE_TYPE (repl)))
> + if (tp != orig_tp)
> + {
> + repl = build_fold_addr_expr (repl);
> + gimple stmt
> + = gimple_build_assign (make_ssa_name (TREE_TYPE (repl), NULL), repl);
> + repl = gimple_assign_lhs (stmt);
> + gimple_stmt_iterator gsi = gsi_for_stmt (info->stmt);
> + gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
> + *orig_tp = repl;
> + }
> + else if (!useless_type_conversion_p (TREE_TYPE (*tp), TREE_TYPE (repl)))
> {
> tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*tp), repl);
> *tp = vce;
> @@ -11328,8 +11336,6 @@ ipa_simd_modify_stmt_ops (tree *tp, int
> *tp = repl;
>
> info->modified = true;
> - wi->is_lhs = false;
> - wi->val_only = true;
> return NULL_TREE;
> }
>
> @@ -11348,7 +11354,7 @@ ipa_simd_modify_function_body (struct cg
> tree retval_array, tree iter)
> {
> basic_block bb;
> - unsigned int i, j;
> + unsigned int i, j, l;
>
> /* Re-use the adjustments array, but this time use it to replace
> every function argument use to an offset into the corresponding
> @@ -11371,6 +11377,46 @@ ipa_simd_modify_function_body (struct cg
> j += node->simdclone->simdlen / TYPE_VECTOR_SUBPARTS (vectype) - 1;
> }
>
> + l = adjustments.length ();
> + for (i = 1; i < num_ssa_names; i++)
> + {
> + tree name = ssa_name (i);
> + if (name
> + && SSA_NAME_VAR (name)
> + && TREE_CODE (SSA_NAME_VAR (name)) == PARM_DECL)
> + {
> + for (j = 0; j < l; j++)
> + if (SSA_NAME_VAR (name) == adjustments[j].base
> + && adjustments[j].new_decl)
> + {
> + tree base_var;
> + if (adjustments[j].new_ssa_base == NULL_TREE)
> + {
> + base_var
> + = copy_var_decl (adjustments[j].base,
> + DECL_NAME (adjustments[j].base),
> + TREE_TYPE (adjustments[j].base));
> + adjustments[j].new_ssa_base = base_var;
> + }
> + else
> + base_var = adjustments[j].new_ssa_base;
> + if (SSA_NAME_IS_DEFAULT_DEF (name))
> + {
> + bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> + gimple_stmt_iterator gsi = gsi_after_labels (bb);
> + tree new_decl = unshare_expr (adjustments[j].new_decl);
> + set_ssa_default_def (cfun, adjustments[j].base, NULL_TREE);
> + SET_SSA_NAME_VAR_OR_IDENTIFIER (name, base_var);
> + SSA_NAME_IS_DEFAULT_DEF (name) = 0;
> + gimple stmt = gimple_build_assign (name, new_decl);
> + gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
> + }
> + else
> + SET_SSA_NAME_VAR_OR_IDENTIFIER (name, base_var);
> + }
> + }
> + }
> +
> struct modify_stmt_info info;
> info.adjustments = adjustments;
>
> --- gcc/tree-dfa.c.jj 2014-03-13 20:09:18.000000000 +0100
> +++ gcc/tree-dfa.c 2014-04-18 11:52:41.043248454 +0200
> @@ -343,7 +343,7 @@ set_ssa_default_def (struct function *fn
> {
> loc = htab_find_slot_with_hash (DEFAULT_DEFS (fn), &in,
> DECL_UID (var), NO_INSERT);
> - if (*loc)
> + if (loc)
> {
> SSA_NAME_IS_DEFAULT_DEF (*(tree *)loc) = false;
> htab_clear_slot (DEFAULT_DEFS (fn), loc);
> --- gcc/testsuite/c-c++-common/gomp/pr60823-1.c.jj 2014-04-17
> 15:35:52.253884292 +0200
> +++ gcc/testsuite/c-c++-common/gomp/pr60823-1.c 2014-04-17
> 15:35:52.253884292 +0200
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/60823 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fopenmp-simd" } */
> +
> +#pragma omp declare simd simdlen(4) notinbranch
> +int
> +foo (const double c1, const double c2)
> +{
> + double z1 = c1, z2 = c2;
> + int res = 100, i;
> +
> + for (i = 0; i < 100; i++)
> + {
> + res = (z1 * z1 + z2 * z2 > 4.0) ? (i < res ? i : res) : res;
> + z1 = c1 + z1 * z1 - z2 * z2;
> + z2 = c2 + 2.0 * z1 * z2;
> + }
> + return res;
> +}
> --- gcc/testsuite/c-c++-common/gomp/pr60823-2.c.jj 2014-04-17
> 15:35:52.254884210 +0200
> +++ gcc/testsuite/c-c++-common/gomp/pr60823-2.c 2014-04-17
> 15:35:52.253884292 +0200
> @@ -0,0 +1,43 @@
> +/* PR tree-optimization/60823 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fopenmp-simd" } */
> +
> +#pragma omp declare simd simdlen(4) notinbranch
> +__attribute__((noinline)) int
> +foo (double c1, double c2)
> +{
> + double z1 = c1, z2 = c2;
> + int res = 100, i;
> +
> + for (i = 0; i < 5; i++)
> + {
> + res = (z1 * z1 + z2 * z2 > 4.0) ? (i < res ? i : res) : res;
> + z1 = c1 + z1 * z1 - z2 * z2;
> + z2 = c2 + 2.0 * z1 * z2;
> + c1 += 0.5;
> + c2 += 0.5;
> + }
> + return res;
> +}
> +
> +__attribute__((noinline, noclone)) void
> +bar (double *x, double *y)
> +{
> + asm volatile ("" : : "rm" (x), "rm" (y) : "memory");
> +}
> +
> +int
> +main ()
> +{
> + int i;
> + double c[4] = { 0.0, 1.0, 0.0, 1.0 };
> + double d[4] = { 0.0, 1.0, 2.0, 0.0 };
> + int e[4];
> + bar (c, d);
> +#pragma omp simd safelen(4)
> + for (i = 0; i < 4; i++)
> + e[i] = foo (c[i], d[i]);
> + if (e[0] != 3 || e[1] != 1 || e[2] != 1 || e[3] != 2)
> + __builtin_abort ();
> + return 0;
> +}
> --- gcc/testsuite/c-c++-common/gomp/pr60823-3.c.jj 2014-04-17
> 16:43:42.580699699 +0200
> +++ gcc/testsuite/c-c++-common/gomp/pr60823-3.c 2014-04-17
> 16:42:59.000000000 +0200
> @@ -0,0 +1,32 @@
> +/* PR tree-optimization/60823 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fopenmp-simd -fno-strict-aliasing" } */
> +
> +void bar (char *, double *);
> +
> +#if __SIZEOF_DOUBLE__ >= 4
> +
> +struct S { char c[sizeof (double)]; };
> +void baz (struct S, struct S);
> +union U { struct S s; double d; };
> +
> +#pragma omp declare simd simdlen(4) notinbranch
> +__attribute__((noinline)) int
> +foo (double c1, double c2)
> +{
> + double *a = &c1;
> + char *b = (char *) &c1 + 2;
> +
> + b[-2]++;
> + b[1]--;
> + *a++;
> + c2++;
> + bar ((char *) &c2 + 1, &c2);
> + c2 *= 3.0;
> + bar (b, a);
> + baz (((union U) { .d = c1 }).s, ((union U) { .d = c2 }).s);
> + baz (*(struct S *)&c1, *(struct S *)&c2);
> + return c1 + c2 + ((struct S *)&c1)->c[1];
> +}
> +
> +#endif
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
