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

[Jakub Jelinek <jakub@redhat.com>]

On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:
> It looks like the C++ bits are quite similar to the C ones.  AFAICT, only
> numbers are allowed for the sink offsets, so no C++ iterators, which would
> likely complicate matters.  If they are eventually allowed, we can implement
> them as a follow up.
> 
> The attached patch addresses all your concerns plus includes the C++
> implementation.  The included test passes for both languages.
> 
> I can work on Fortran next if you'd like.

Please leave Fortran unresolved for now, we'll see in Autumn if we have time
for Fortran OpenMP 4.1 support, or not, there is also the possibility to
handle it like in 4.9 - 4.9.0 came with just C/C++ OpenMP 4.0 support
(and Fortran only OpenMP 3.1 support) and 4.9.1 added Fortran OpenMP 4.0 support.

Please write ChangeLog entries and commit them into */ChangeLog.gomp files.

> +	  if (c_parser_next_token_is_not (parser, CPP_NUMBER))
> +	    {
> +	      c_parser_error (parser, "expected %<integer%>");

I think %< and %> here

> +	      return list;
> +	    }
> +
> +	  addend = c_parser_peek_token (parser)->value;
> +	  if (TREE_CODE (addend) != INTEGER_CST)
> +	    {
> +	      c_parser_error (parser, "expected %<integer%>");

and here aren't appropriate here, you don't expect integer as a keyword,
but some integer...

On the C++ FE side, please also try a testcase in g++.dg/gomp/ where
the ordered(n) loop with #pragma omp ordered depend({source,sink}) will be
in a template, to make sure pt.c does the right thing with it.

> +	  if (cp_lexer_next_token_is_not (parser->lexer, CPP_NUMBER))
> +	    {
> +	      cp_parser_error (parser, "expected %<integer%>");
> +	      return list;
> +	    }
> +
> +	  addend = cp_lexer_peek_token (parser->lexer)->u.value;
> +	  if (TREE_CODE (addend) != INTEGER_CST)
> +	    {
> +	      cp_parser_error (parser, "expected %<integer%>");

See above.

> @@ -365,6 +367,8 @@ new_omp_context (enum omp_region_type region_type)
>  
>    c = XCNEW (struct gimplify_omp_ctx);
>    c->outer_context = gimplify_omp_ctxp;
> +  c->iter_vars.safe_push(0);
> +  c->iter_vars.pop();

As mentioned, please leave this out.

> @@ -8982,7 +8997,36 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>  		}
>  		break;
>  	      case OMP_ORDERED:
> -		g = gimple_build_omp_ordered (body);
> +		if (gimplify_omp_ctxp)
> +		  for (tree c = OMP_ORDERED_CLAUSES (*expr_p);
> +		       c; c = OMP_CLAUSE_CHAIN (c))
> +		    if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			&& OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
> +		      {
> +			unsigned int n = 0;
> +			bool fail = false;
> +			for (tree decls = OMP_CLAUSE_DECL (c);
> +			     decls && TREE_CODE (decls) == TREE_LIST;
> +			     decls = TREE_CHAIN (decls), ++n)
> +			  if (n < gimplify_omp_ctxp->iter_vars.length ()
> +			      && TREE_VALUE (decls)
> +			      != gimplify_omp_ctxp->iter_vars[n])
> +			    {
> +			      error_at (OMP_CLAUSE_LOCATION (c),
> +					"variable %qE is not an iteration "
> +					"variable", TREE_VALUE (decls));

I think this error message will be confusing to users, if they write
#pragma omp for ordered(3)
for (int i = 0; i < 10; i++)
for (int j = 0; j < 10; j++)
for (int k = 0; k < 10; k++)
{
#pragma omp ordered depend(sink:k-1, j+2, i-3)
#pragma omp ordered depend(source)
}
because then it will complain that k and i are not iteration
variables, when they in fact are, just in wrong order.

I believe our diagnostics doesn't have support for ngettext style
of diagnostic messages (1st vs. 2nd, 3rd, 4th ...); I wonder if
saying variable %qE is not an iteration variable of outermost loop %d, expected %qE",
TREE_VALUE (decls), n + 1, gimplify_omp_ctxp->iter_vars[n]
wouldn't be better or something similar.

> +			      fail = true;
> +			    }
> +			/* Avoid being too redundant.  */
> +			if (!fail
> +			    && n != gimplify_omp_ctxp->iter_vars.length ())
> +			  error_at (OMP_CLAUSE_LOCATION (c),
> +			     "number of variables in depend(sink) clause "
> +			     "does not match number of iteration variables");
> +		      }
> +
> +		g = gimple_build_omp_ordered (body,
> +					      OMP_ORDERED_CLAUSES (*expr_p));
>  		break;
>  	      case OMP_CRITICAL:
>  		gimplify_scan_omp_clauses (&OMP_CRITICAL_CLAUSES (*expr_p),
> diff --git a/gcc/omp-low.c b/gcc/omp-low.c
> index 83677ea..3dec095 100644
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -2996,6 +2996,8 @@ scan_omp_teams (gomp_teams *stmt, omp_context *outer_ctx)
>  static bool
>  check_omp_nesting_restrictions (gimple stmt, omp_context *ctx)
>  {
> +  tree c;
> +
>    /* No nesting of non-OpenACC STMT (that is, an OpenMP one, or a GOMP builtin)
>       inside an OpenACC CTX.  */
>    if (!(is_gimple_omp (stmt)
> @@ -3216,7 +3218,54 @@ 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))
> +	  {
> +	    enum omp_clause_depend_kind kind = OMP_CLAUSE_DEPEND_KIND (c);
> +	    gcc_assert (kind == OMP_CLAUSE_DEPEND_SOURCE
> +			|| kind == OMP_CLAUSE_DEPEND_SINK);
> +	    error_at (OMP_CLAUSE_LOCATION (c),
> +		      "depend(%s) is only available in %<omp ordered%>",

%<depend(%s)%> ?  Also, I'd perhaps replace available with allowed.

> +	      /* Look for containing ordered(N) loop.  */
> +	      for (omp_context *octx = ctx; octx; octx = octx->outer)
> +		if (gimple_code (octx->stmt) == GIMPLE_OMP_FOR
> +		    && find_omp_clause (gimple_omp_for_clauses (octx->stmt),
> +					OMP_CLAUSE_ORDERED))

I think you want to save the result of find_omp_clause in a temporary
and also test if OMP_CLAUSE_ORDERED_EXPR (c) != NULL_TREE.


> +		  {
> +		    have_ordered = true;
> +		    break;
> +		  }
> +	      if (!have_ordered)
> +		{
> +		  error_at (OMP_CLAUSE_LOCATION (c),
> +			    "depend clause must be closely nested inside an "

%<depend%> ?

If you want to spend time on something still in the FE, it would be nice to
resolve the C++ iteration var issue (i.e. increase OMP_FOR number of
arguments, so that it could have yet another (optional) vector, say
OMP_FOR_ORIG_DECLS.  If that vector would be NULL, the gimplifier would
assume that all the decls in OMP_FOR_INIT are the ones present in the
source, if it would be present, you'd use them for the variable checking
instead of the ones from OMP_FOR_INIT (but, replace them with the
decls from OMP_FOR_INIT after the checking).

There is another issue - if some iterator var has pointer type, supposedly
we want somewhere in the FEs already multiply it by the size of what they
point to (and convert to sizetype).  For C FE, it can be done already during
parsing, we should know the type of the iterator var already at that point,
for C++ FE it needs to be done only in finish_omp_clauses if
!processing_template_decl, because in templates we might not know the type.

	Jakub