Re: [gomp4.1] fold ordered depend(sink) clauses

[Jakub Jelinek <jakub at redhat dot com>]

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