On Thu, Jul 30, 2015 at 07:27:51PM -0700, Aldy Hernandez wrote:
> +static gimple
> +gimplify_omp_ordered (tree expr, gimple_seq body)
> +{
> + tree c, decls;
> + int failures = 0;
> + unsigned int i;
> +
> + if (gimplify_omp_ctxp)
> + for (c = OMP_ORDERED_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c))
> + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> + && OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
> + {
> + bool fail = false;
> + for (decls = OMP_CLAUSE_DECL (c), i = 0;
> + decls && TREE_CODE (decls) == TREE_LIST;
> + decls = TREE_CHAIN (decls), ++i)
> + if (i < gimplify_omp_ctxp->loop_iter_var.length ()
> + && TREE_VALUE (decls) != gimplify_omp_ctxp->loop_iter_var[i])
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "variable %qE is not an iteration "
> + "of outermost loop %d, expected %qE",
> + TREE_VALUE (decls), i + 1,
> + gimplify_omp_ctxp->loop_iter_var[i]);
> + fail = true;
> + failures++;
> + }
> + /* Avoid being too redundant. */
> + if (!fail && i != gimplify_omp_ctxp->loop_iter_var.length ())
> + {
> + error_at (OMP_CLAUSE_LOCATION (c),
> + "number of variables in depend(sink) "
> + "clause does not match number of "
> + "iteration variables");
> + failures++;
> + }
failures seems to be a write only variable.
Perhaps if fail is true (set it to true after this error too),
don't create the ordered at all? Or drop the bogus clauses.
> +
> + /* ?? This is stupid. We need to call extract_omp_for_data just
> + to get the number of ordered loops... */
> + extract_omp_for_data (as_a <gomp_for *> (ctx->outer->stmt), &fd, NULL);
> + if (!fd.ordered)
> + return;
> + struct omp_for_data_loop *loops
> + = (struct omp_for_data_loop *)
> + alloca (fd.ordered * sizeof (struct omp_for_data_loop));
You can do just what expand_omp_for does:
struct omp_for_data fd;
struct omp_for_data_loop *loops;
loops
= (struct omp_for_data_loop *)
alloca (gimple_omp_for_collapse (ctx->outer->stmt)
* sizeof (struct omp_for_data_loop));
extract_omp_for_data (as_a <gomp_for *> (ctx->outer_stmt), &fd, loops);
> + For example:
> +
> + for (i=0; i < N; ++i)
> + depend(sink:i-8,j-1)
> + depend(sink:i,j-2) // Completely ignored because i+0.
> + depend(sink:i-4,j+3)
> + depend(sink:i-6,j+2)
Even when writing comments, it is better to make it valid:
#pragma omp for ordered(2)
for (i=0; i < N; ++i)
for (j=0; j < M; ++j)
{
#pragma omp ordered \
depend(sink:i-8,j-1) \
depend(sink:i,j-2) \ // Completely ignored because i+0.
depend(sink:i-4,j+3) \
depend(sink:i-6,j+2)
#pragma omp ordered depend(source)
}
> +
> + Folded clause is:
> +
> + depend(sink:-gcd(8,4,6),min(-1,3,2))
Spaces here instead of desirable tab.
> + -or-
> + depend(sink:-2,-1)
> + */
> +
> + /* FIXME: Computing GCD's where the first element is zero is
> + non-trivial in the presence of collapsed loops. Do this later. */
> + if (fd.collapse > 1)
> + return;
Better ad an gcc_assert for now.
> + enum {
> + DIR_UNKNOWN,
> + DIR_FORWARD,
> + DIR_BACKWARD
> + } loop_dir;
> + switch (fd.loops[i].cond_code)
> + {
> + case LT_EXPR:
> + case LE_EXPR:
> + loop_dir = DIR_FORWARD;
> + break;
> + case GT_EXPR:
> + case GE_EXPR:
> + loop_dir = DIR_BACKWARD;
> + break;
> + default:
> + loop_dir = DIR_UNKNOWN;
> + gcc_unreachable ();
> + }
I think there is no point in doing this, extract_omp_for_data
canonicalizes cond_code already, so it is always
LT_EXPR or GT_EXPR. So just gcc_assert it is these two and
compare it directly, or add bool forward = fd.loops[i].cond_code == LT_EXPR;
?
> +
> + /* While the committee makes up its mind, bail if we have any
> + non-constant steps. */
> + if (TREE_CODE (fd.loops[i].step) != INTEGER_CST)
> + goto gimplify_omp_ordered_ret;
Misnamed label. lower instead?
> + wide_int offset = TREE_PURPOSE (decls);
> + if (!iter_vars[i])
> + iter_vars[i] = TREE_VALUE (decls);
> +
> + /* Ignore invalid offsets that are not multiples of the step. */
> + if (!wi::multiple_of_p
> + (wi::abs (offset), wi::abs ((wide_int) fd.loops[i].step),
> + UNSIGNED))
What does wi::abs do with very large unsigned integers?
#pragma omp ordered(2)
for (unsigned int i = 64; i > 32; i += -1)
for (unsigned int j = 0; j < 32; j++)
?
step of i is 0xffffffff and cond_code is GT_EXPR. I suppose for unsigned
steps you don't want to use wi::abs, but just negate the step if
iterating downward?
Jakub
