Re: [Patch] OpenMP: Add iterator support to Fortran's depend; add affinity clause

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Apr 27, 2021 at 03:36:38PM +0200, Tobias Burnus wrote:
> OpenMP 5's iterator can be used for
> - depend clause
> - affinity clause
> - mapping (unsupported and not touched)
> 
> (a) This patch add the iterator support to the Fortran FE
> and adds support for it to the depend clause.
> 
> (b) It also adds a conforming stub implementation (parse & ignore in ME)
>   for 'affinity' (Fortran, C, C++)
> 
> (c) Fortran's taskwait did not handle the depend clause, now it does.
> 
> The way the iterator is stored in Fortran is a bit convoluted,
> but should be fine:
> new namespace (such that symbols can be overridden and resolution works),
> using symbol->value for the iteration (begin:end:step) as array constructor,
> and abusing the ns->proc_name + its '->tlink' to generate an linked list,
> which avoids walking the ns->sym_root tree and has the user's order in
> the dump instead of the tree-walking order.
> 
> The ME implementation also seems to require a very special way the
> variables are stored. – It seems as if it works correctly for depend;
> hence, I hope I did correctly read the dump and tree sharing is correctly
> handled.
> 
> NOTE: The iterator Fortran patch includes one change from the post-OpenMP-5.0 spec:
> The '::' after the typespec in order to avoid Fortran free-form source issues with:

Is the :: optional or required in free-form?

> 'iterator(integer i=1:10)' – namely: is this the integer 'i' or the variable
> 'integeri' (as spaces are ignored in fixed-form Fortran)?
> NOTE 2: The way it is implemented, the 'begin:end:step' expression is evaluated
> multiple times - once per list item; I think this matches C, but I am not completely
> sure nor whether that is a problem. (Unlikely a real-world problem.)

I believe C evaluates them just once and it would be nice if that could be
the case for Fortran too.
At least, looking at:
int bar (int);
void
foo (void)
{
  int a[64];
#pragma omp task depend (iterator (j=bar(0):bar(1):bar(2)) , out : a[j])
  ;
}
I see all the bar calls just once:
  saved_stack.6 = __builtin_stack_save ();
  try
    {
      _1 = bar (0);
      _2 = bar (1);
      D.2078 = bar (2);
and never afterwards.  So, it would be nice if Fortran could do the same,
wrap stuff in SAVE_EXPR or whatever is needed for that.
And even
void
qux (void)
{
  int a[64], b[64], c[64];
#pragma omp task depend (iterator (j=bar(0):bar(1):bar(2)) , out : a[j], b[j], c[j])
  ;
}
seems to evaluate just once.

> NOTE 3: I did have some trouble reading the spec with regards to what in C is permitted
> for 'affinity' ('locator-list); the code now mostly follows what is permitted for 'depend'.

locator-list is used for both affinity and depend, so I guess the same.
I think at least for the latter C/C++ arranges that by using the same
iterator tree for the adjacent vars, so that the middle-end knows it can
just evaluate it once and emit one loop instead of one loop for each
variable separately.  Of course, depend (iterator (...),out : a[j]) depend(iterator (...),out : b[j])
will emit two different loops.

> @@ -15508,6 +15511,52 @@ c_parser_omp_iterators (c_parser *parser)
>    return ret ? ret : error_mark_node;
>  }
>  
> +/* OpenMP 5.0:
> +   affinity( [depend-modifier :] variable-list)

For consistency, I think the syntax for other clauses puts space
before ( and before ) too.
Also, s/depend-modifier/aff-modifier/

> +   depend-modifier:

Likewise.

> +     iterator ( iterators-definition )  */
> +
> +static tree
> +c_parser_omp_clause_affinity (c_parser *parser, tree list)
> +{
> +  location_t clause_loc = c_parser_peek_token (parser)->location;
> +  tree nl, iterators = NULL_TREE;
> +
> +  matching_parens parens;
> +  if (!parens.require_open (parser))
> +    return list;
> +
> +  if (c_parser_next_token_is (parser, CPP_NAME))
> +    {
> +      const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
> +      if (strcmp ("iterator", p) == 0)
> +	{
> +	  iterators = c_parser_omp_iterators (parser);
> +	   if (!c_parser_require (parser, CPP_COLON, "expected %<:%>"))
> +	     return list;

I think in this case it should
  if (iterators)
    pop_scope ();
  parens.skip_until_found_close (parser);
before doing return list;

> +	}
> +    }
> +  nl = c_parser_omp_variable_list (parser, clause_loc, OMP_CLAUSE_AFFINITY,
> +				   list);
> +  if (iterators)
> +    {
> +      tree block = pop_scope ();
> +      if (iterators == error_mark_node)
> +	iterators = NULL_TREE;
> +      else
> +	{
> +	  TREE_VEC_ELT (iterators, 5) = block;
> +	  for (tree c = nl; c != list; c = OMP_CLAUSE_CHAIN (c))
> +	    OMP_CLAUSE_DECL (c) = build_tree_list (iterators,
> +						   OMP_CLAUSE_DECL (c));
> +	}
> +    }
> +
> +  parens.skip_until_found_close (parser);
> +  return nl;
> +}
> +
> +

> @@ -14578,17 +14592,24 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  	    {
>  	      error_at (OMP_CLAUSE_LOCATION (c),
>  			"%qE is not lvalue expression nor array section in "
> -			"%<depend%> clause", t);
> +			"%qs clause", t,
> +			OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			? "depend" : "affinity");

You could just use omp_clause_code_name[OMP_CLAUSE_CODE (c)] here.

>  	      remove = true;
>  	    }
>  	  else if (TREE_CODE (t) == COMPONENT_REF
>  		   && DECL_C_BIT_FIELD (TREE_OPERAND (t, 1)))
>  	    {
> +	      gcc_assert (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			  || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY);
>  	      error_at (OMP_CLAUSE_LOCATION (c),
> -			"bit-field %qE in %qs clause", t, "depend");
> +			"bit-field %qE in %qs clause", t,
> +			OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			? "depend" : "affinity");

Likewise.

> @@ -37707,6 +37710,56 @@ cp_parser_omp_iterators (cp_parser *parser)
>    return ret ? ret : error_mark_node;
>  }
>  
> +/* OpenMP 5.0:
> +   affinity( [depend-modifier :] variable-list)
> +   depend-modifier:
> +     iterator ( iterators-definition )  */

See above.

> +
> +static tree
> +cp_parser_omp_clause_affinity (cp_parser *parser, tree list)
> +{
> +  tree nlist, c, iterators = NULL_TREE;
> +
> +  matching_parens parens;
> +  if (!parens.require_open (parser))
> +    return list;
> +
> +  if (cp_lexer_next_token_is (parser->lexer, CPP_NAME))
> +    {
> +      tree id = cp_lexer_peek_token (parser->lexer)->u.value;
> +      const char *p = IDENTIFIER_POINTER (id);
> +      if (strcmp ("iterator", p) == 0)
> +	{
> +	  begin_scope (sk_omp, NULL);
> +	  iterators = cp_parser_omp_iterators (parser);
> +	  if (!cp_parser_require (parser, CPP_COLON, RT_COLON))
> +	    {

Again, missing
  if (iterators)
    poplevel (0, 1, 0);
here.

> +	      cp_parser_skip_to_closing_parenthesis (parser,
> +						     /*recovering=*/true,
> +						     /*or_comma=*/false,
> +						     /*consume_paren=*/true);
> +	      return list;
> +	    }
> +	}
> +    }

> @@ -7461,7 +7471,9 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  	    {
>  	      if (handle_omp_array_sections (c, ort))
>  		remove = true;
> -	      else if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_DEPOBJ)
> +	      else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +		       && OMP_CLAUSE_DEPEND_KIND (c)
			  == OMP_CLAUSE_DEPEND_DEPOBJ)

I think ()s should be added (I think at least emacs prefers that) when the
&& rhs doesn't fit on one line.

> @@ -7486,22 +7498,31 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  	      if (DECL_P (t))
>  		error_at (OMP_CLAUSE_LOCATION (c),
>  			  "%qD is not lvalue expression nor array section "
> -			  "in %<depend%> clause", t);
> +			  "in %qs clause", t,
> +			  OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			  ? "depend" : "affinity");
>  	      else
>  		error_at (OMP_CLAUSE_LOCATION (c),
>  			  "%qE is not lvalue expression nor array section "
> -			  "in %<depend%> clause", t);
> +			  "in %qs clause", t,
> +			  OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			  ? "depend" : "affinity");

See above 2x.
>  	      remove = true;
>  	    }
>  	  else if (TREE_CODE (t) == COMPONENT_REF
>  		   && TREE_CODE (TREE_OPERAND (t, 1)) == FIELD_DECL
>  		   && DECL_BIT_FIELD (TREE_OPERAND (t, 1)))
>  	    {
> +	      gcc_assert (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			  || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_AFFINITY);
>  	      error_at (OMP_CLAUSE_LOCATION (c),
> -			"bit-field %qE in %qs clause", t, "depend");
> +			"bit-field %qE in %qs clause", t,
> +			OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
> +			? "depend" : "affinity");

And again.

> @@ -261,6 +263,7 @@ gfc_match_omp_variable_list (const char *str, gfc_omp_namelist **list,
>  	case MATCH_YES:
>  	  gfc_expr *expr;
>  	  expr = NULL;
> +	  gfc_gobble_whitespace ();
>  	  if ((allow_sections && gfc_peek_ascii_char () == '(')
>  	      || (allow_derived && gfc_peek_ascii_char () == '%'))
>  	    {

Is this change specific to depend/affinity or iterators?
If not, shouldn't it go in separately and with a testcase that shows when it
is needed?

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -9506,6 +9506,10 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
>  
>  	  goto do_add;
>  
> +	case OMP_CLAUSE_AFFINITY:
> +	  /* Ignore.  */
> +	  remove = true;
> +	  break;
>  	case OMP_CLAUSE_DEPEND:
>  	  if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
>  	    {

I think removing affinity at this point is probably too early.
That will mean we e.g. won't diagnose invalid programs that e.g. use
default(none) on some outer OpenMP construct and then refer to variables
that aren't mentioned in any of the parallel/host teams clauses with such
default(none).  We don't need to emit the loops we emit for depend, sure
(though if it is easier we could and hope for it to be optimized away).
Or at least we should evaluate the iterator expressions for side-effects
(i.e. gimplify them and ignore) and probably gimplify the affinity vars
too.  If we emit the loops, just gimplify the location-list items in the
loop and then throw them away, if not, evaluate it like
step; iter = beg; if (iter cond end) expr; or so (so that expr isn't evaluated
if the cond isn't true and all of beg, end and step are evaluated.

> diff --git a/gcc/testsuite/c-c++-common/gomp/affinity-1.c b/gcc/testsuite/c-c++-common/gomp/affinity-1.c
> new file mode 100644
> index 00000000000..558e316bccd
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/gomp/affinity-1.c
> @@ -0,0 +1,20 @@
> +void
> +foo(int x)
> +{ 
> +  int a, b[5], cc, d[5][5];

Can you please initialize the vars (at least those that are actually used
like a)?

> +#pragma omp taskgroup
> + {
> +  #pragma omp task affinity(a)
> +  { }
> +  #pragma omp task affinity(iterator(i=(int)__builtin_cos(1.0+a):5, jj =2:5:2) : b[i], d[i][jj])
> +  { }
> +  #pragma omp task affinity(iterator(i=(int)__builtin_cos(1.0+a):5) : b[i], d[i][i])
> +  { }
> +  #pragma omp task affinity (iterator(i=1:5): a)
> +  { }
> +  #pragma omp task affinity (iterator(i=1:5): a) affinity(iterator(i=1:5) : x)
> +  { }
> +  #pragma omp task affinity (iterator(unsigned long j=1:5, k=7:4:-1) : b[j+k],a) affinity (cc)
> +  { }

Perhaps just ; instead of { }, your choice.

	Jakub