Re: [gomp4.1] depend(sink) and depend(source) parsing for C

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

On Thu, Jul 09, 2015 at 11:24:44AM -0700, Aldy Hernandez wrote:

Thanks for working on it.

> +	      wide_int offset = wi::neg (addend, &overflow);
> +	      addend = wide_int_to_tree (TREE_TYPE (addend), offset);
> +	      if (overflow)
> +		warning_at (c_parser_peek_token (parser)->location,
> +			    OPT_Woverflow,
> +			    "possible overflow in %<depend(sink)%> offset");

possible overflow looks weird.  Shouldn't it complain the same
as it does if you do:
int c = - (-2147483648);
?

> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool declare_simd)
>  			  == OMP_CLAUSE_DEPEND_SOURCE);
>  	      break;
>  	    }
> +	  if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
> +	    {
> +	      gcc_assert (TREE_CODE (t) == TREE_LIST);
> +	      break;
> +	    }
>  	  if (TREE_CODE (t) == TREE_LIST)
>  	    {
>  	      if (handle_omp_array_sections (c))

Won't this ICE if somebody uses depend(sink:) ? or depend(sink:.::) or
similar garbage?  Make sure you don't create OMP_CLAUSE_DEPEND in that
case.

> diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
> index f0e2c67..ba79977 100644
> --- a/gcc/gimple-walk.c
> +++ b/gcc/gimple-walk.c
> @@ -327,6 +327,10 @@ walk_gimple_op (gimple stmt, walk_tree_fn callback_op,
>        }
>        break;
>  
> +    case GIMPLE_OMP_ORDERED:
> +      /* Ignore clauses.  */
> +      break;
> +

I'm not convinced you don't want to walk the clauses.

> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index 6057ea0..e33fe1e 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -527,6 +527,17 @@ struct GTY((tag("GSS_OMP_CRITICAL")))
>    tree name;
>  };
>  
> +/* GIMPLE_OMP_ORDERED */
> +
> +struct GTY((tag("GSS_OMP_ORDERED")))
> +  gomp_ordered : public gimple_statement_omp
> +{
> +  /* [ WORD 1-7 ] : base class */
> +
> +  /* [ WORD 8 ]  */
> +  tree clauses;
> +};

I would have expected to use 
struct GTY((tag("GSS_OMP_SINGLE_LAYOUT")))
  gomp_ordered : public gimple_statement_omp_single_layout
{
    /* No extra fields; adds invariant:
         stmt->code == GIMPLE_OMP_ORDERED.  */
};
instead (like gomp_single, gomp_teams, ...).

> @@ -149,6 +149,9 @@ struct gimplify_omp_ctx
>    struct gimplify_omp_ctx *outer_context;
>    splay_tree variables;
>    hash_set<tree> *privatized_types;
> +  /* Iteration variables in an OMP_FOR.  */
> +  tree *iter_vars;
> +  int niter_vars;

Wonder if it wouldn't be better to use a vec<tree> instead.
Then the size would be there as vec_length.

> @@ -8169,6 +8185,19 @@ gimplify_transaction (tree *expr_p, gimple_seq *pre_p)
>    return GS_ALL_DONE;
>  }
>  
> +/* Verify the validity of the depend(sink:...) variable VAR.
> +   Return TRUE if everything is OK, otherwise return FALSE.  */
> +
> +static bool
> +verify_sink_var (location_t loc, tree var)
> +{
> +  for (int i = 0; i < gimplify_omp_ctxp->niter_vars; ++i)
> +    if (var == gimplify_omp_ctxp->iter_vars[i])
> +      return true;
> +  error_at (loc, "variable %qE is not an iteration variable", var);
> +  return false;

I believe what we want to verify is that ith variable in the OMP_CLAUSE_DECL
vector is iter_vars[i], so not just some random permutation etc.

> @@ -3216,7 +3218,51 @@ check_omp_nesting_restrictions (gimple stmt, omp_context *ctx)
>  	    break;
>  	  }
>        break;
> +    case GIMPLE_OMP_TASK:
> +      for (c = gimple_omp_task_clauses (stmt); c; c = OMP_CLAUSE_CHAIN (c))
> +	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +	    && (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
> +		|| OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK))
> +	  {
> +	    error_at (OMP_CLAUSE_LOCATION (c),
> +		      "depend(%s) is only available in 'omp ordered'",

Please avoid using ' in diagnostics, it should be %<omp ordered%> instead.

> +		      OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
> +		      ? "source" : "sink");
> +	    return false;
> +	  }
> +      break;

This will eventually be needed also for GIMPLE_OMP_TARGET and
GIMPLE_OMP_ENTER/EXIT_DATA.  But as that isn't really supported right now,
can wait.

>      case GIMPLE_OMP_ORDERED:
> +      for (c = gimple_omp_ordered_clauses (as_a <gomp_ordered *> (stmt));
> +	   c; c = OMP_CLAUSE_CHAIN (c))
> +	{
> +	  enum omp_clause_depend_kind kind = OMP_CLAUSE_DEPEND_KIND (c);
> +	  if (kind == OMP_CLAUSE_DEPEND_SOURCE
> +	      || kind == OMP_CLAUSE_DEPEND_SINK)
> +	    {
> +	      bool have_ordered = false;
> +	      /* Look for containing ordered(N) loop.  */
> +	      for (omp_context *ctx_ = ctx; ctx_; ctx_ = ctx_->outer)

Please use octx or something similar, I don't like the trailing _ ;)

> +	      if (!have_ordered)
> +		{
> +		  error_at (OMP_CLAUSE_LOCATION (c),
> +			    "depend clause is not within an ordered loop");

Not within is not the right OpenMP term, the requirement is that it must
be closely nested in ordered loop.

> +/* depend(sink...) is allowed without an offset.  */
> +#pragma omp ordered depend(sink:i,j+1)

Can you write depend(sink:i,j-1) at least?  The iteration to depend
on must be lexicographically earlier in the loop.

> +#pragma omp ordered depend(sink:i+2,j-2,k+2) /* { dg-error "is not an iteration var" } */

Similarly.  i-2 will be enough.

> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -1479,7 +1479,7 @@ remap_gimple_stmt (gimple stmt, copy_body_data *id)
>  
>  	case GIMPLE_OMP_ORDERED:
>  	  s1 = remap_gimple_seq (gimple_omp_body (stmt), id);
> -	  copy = gimple_build_omp_ordered (s1);
> +	  copy = gimple_build_omp_ordered (s1, NULL);

You surely don't want to pass NULL here, I bet you want
gimple_omp_ordered_clauses (stmt) instead.

> --- a/gcc/tree-pretty-print.c
> +++ b/gcc/tree-pretty-print.c
> @@ -533,6 +533,9 @@ dump_omp_clause (pretty_printer *pp, tree clause, int spc, int flags)
>  	case OMP_CLAUSE_DEPEND_SOURCE:
>  	  pp_string (pp, "source)");
>  	  return;
> +	case OMP_CLAUSE_DEPEND_SINK:
> +	  pp_string (pp, "sink");
> +	  break;

And here you surely don't want to emit
#pragma omp ordered(sink
(note even the missing closing paren).
It should dump the TREE_LIST (the var and if non-0 addend, the addend after
it).

	Jakub