Re: [patch] various OpenACC reduction enhancements - FE changes

[Jakub Jelinek <jakub at redhat dot com>]

On Fri, Jun 29, 2018 at 11:22:00AM -0700, Cesar Philippidis wrote:
> 2018-06-29  Cesar Philippidis  <cesar@codesourcery.com>
> 	    Nathan Sidwell  <nathan@acm.org>
> 
> 	gcc/c/
> 	* c-parser.c (c_parser_omp_variable_list): New c_omp_region_type
> 	argument.  Use it to specialize handling of OMP_CLAUSE_REDUCTION for
> 	OpenACC.
> 	(c_parser_omp_clause_reduction): Update call to
> 	c_parser_omp_variable_list.  Propage OpenACC errors as necessary.
> 	(c_parser_oacc_all_clauses): Update call to
> 	p_parser_omp_clause_reduction.
> 	(c_parser_omp_all_clauses): Likewise.
> 	* c-typeck.c (c_finish_omp_clauses): Emit an error on orphan OpenACC
> 	gang reductions.
> 
> 	gcc/cp/
> 	* parser.c (cp_parser_omp_var_list_no_open):  New c_omp_region_type
> 	argument.  Use it to specialize handling of OMP_CLAUSE_REDUCTION for
> 	OpenACC.
> 	(cp_parser_omp_clause_reduction): Update call to
> 	cp_parser_omp_variable_list.  Propage OpenACC errors as necessary.
> 	(cp_parser_oacc_all_clauses): Update call to
> 	cp_parser_omp_clause_reduction.
> 	(cp_parser_omp_all_clauses): Likewise.
> 	* semantics.c (finish_omp_clauses): Emit an error on orphan OpenACC
> 	gang reductions.
> 
> 	gcc/fortran/
> 	* openmp.c (resolve_oacc_loop_blocks): Emit an error on orphan OpenACC
> 	gang reductions.
> 	* trans-openmp.c (gfc_omp_clause_copy_ctor): Permit reductions.
> 
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 7a926285f3a..a6f453dae54 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -965,12 +965,13 @@ class token_pair
>  
>    /* Like token_pair::require_close, except that tokens will be skipped
>       until the desired token is found.  An error message is still produced
> -     if the next token is not as expected.  */
> +     if the next token is not as expected, unless QUIET is set.  */
>  
> -  void skip_until_found_close (c_parser *parser) const
> +  void skip_until_found_close (c_parser *parser, bool quiet = false) const
>    {
>      c_parser_skip_until_found (parser, traits_t::close_token_type,
> -			       traits_t::close_gmsgid, m_open_loc);
> +			       quiet ? NULL : traits_t::close_gmsgid,
> +			       m_open_loc);
>    }

I don't like these changes, why do you need them?  C++ FE doesn't have such
changes either, and it is fine to diagnose missing ) even if there was some
earlier error.  All other spots which require matching parens do it the
same.  Please leave those out.

 static tree                                                                                                                                       
-c_parser_omp_clause_reduction (c_parser *parser, tree list)                                                                                       
+c_parser_omp_clause_reduction (c_parser *parser, tree list,                                                                                       
+                              enum c_omp_region_type ort)                                                                                         

Note, the signature is now different, it is ok to replace is_omp argument
with enum c_omp_region_type if you wish.

>  {
>    location_t clause_loc = c_parser_peek_token (parser)->location;
> +  bool seen_error = false;
> +
>    matching_parens parens;
>    if (parens.require_open (parser))
>      {
> @@ -12855,7 +12876,13 @@ c_parser_omp_clause_reduction (c_parser *parser, tree list)
>  	  tree nl, c;
>  
>  	  nl = c_parser_omp_variable_list (parser, clause_loc,
> -					   OMP_CLAUSE_REDUCTION, list);
> +					   OMP_CLAUSE_REDUCTION, list, ort);
> +	  if (c_parser_peek_token (parser)->type != CPP_CLOSE_PAREN)
> +	    {
> +	      seen_error = true;
> +	      goto cleanup;
> +	    }
> +
>  	  for (c = nl; c != list; c = OMP_CLAUSE_CHAIN (c))
>  	    {
>  	      tree d = OMP_CLAUSE_DECL (c), type;
> @@ -12891,7 +12918,8 @@ c_parser_omp_clause_reduction (c_parser *parser, tree list)
>  
>  	  list = nl;
>  	}
> -      parens.skip_until_found_close (parser);
> +    cleanup:
> +      parens.skip_until_found_close (parser, seen_error);
>      }
>    return list;
>  }

And the above hunks as well.

> @@ -13998,7 +14026,7 @@ c_parser_oacc_all_clauses (c_parser *parser, omp_clause_mask mask,
>  	  c_name = "private";
>  	  break;
>  	case PRAGMA_OACC_CLAUSE_REDUCTION:
> -	  clauses = c_parser_omp_clause_reduction (parser, clauses);
> +	  clauses = c_parser_omp_clause_reduction (parser, clauses, C_ORT_ACC);
>  	  c_name = "reduction";
>  	  break;
>  	case PRAGMA_OACC_CLAUSE_SEQ:
> @@ -14157,7 +14185,7 @@ c_parser_omp_all_clauses (c_parser *parser, omp_clause_mask mask,
>  	  c_name = "private";
>  	  break;
>  	case PRAGMA_OMP_CLAUSE_REDUCTION:
> -	  clauses = c_parser_omp_clause_reduction (parser, clauses);
> +	  clauses = c_parser_omp_clause_reduction (parser, clauses, C_ORT_OMP);
>  	  c_name = "reduction";
>  	  break;
>  	case PRAGMA_OMP_CLAUSE_SCHEDULE:

Note, there are now also the IN_REDUCTION/TASK_REDUCTION clause cases that
need adjustment.

> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 90ae306c99a..944db3fa8be 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -13087,6 +13087,14 @@ c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  	  goto check_dup_generic;
>  
>  	case OMP_CLAUSE_REDUCTION:
> +	  if (ort == C_ORT_ACC && oacc_get_fn_attrib (current_function_decl)
> +	      && omp_find_clause (clauses, OMP_CLAUSE_GANG))

This is expensive if there are many clauses, we want to avoid O(n^2)
behavior.  For C we only have one loop, so just remember in some variable
whether there are reduction clause(s) that would conflict with gang, and
in another one whether gang clause has been seen, then deal with it at the
end if both the bools are true.

> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c),
> +			"gang reduction on an orphan loop");
> +	      remove = true;
> +	      break;
> +	    }
>  	  need_implicitly_determined = true;
>  	  t = OMP_CLAUSE_DECL (c);
>  	  if (TREE_CODE (t) == TREE_LIST)

> @@ -31668,6 +31669,21 @@ cp_parser_omp_var_list_no_open (cp_parser *parser, enum omp_clause_code kind,
>  	      /* FALLTHROUGH.  */
>  	    case OMP_CLAUSE_DEPEND:
>  	    case OMP_CLAUSE_REDUCTION:
> +	      if (kind == OMP_CLAUSE_REDUCTION && ort == C_ORT_ACC)
> +		{
> +		  switch (cp_lexer_peek_token (parser->lexer)->type)
> +		    {
> +		    case CPP_OPEN_PAREN:
> +		    case CPP_OPEN_SQUARE:
> +		    case CPP_DOT:
> +		    case CPP_DEREF:
> +		      error ("invalid reduction variable");
> +		      decl = error_mark_node;
> +		      goto skip_comma;
> +		    default:;
> +		      break;
> +		    }
> +		}

Any reason for the above (ditto in C), rather than just adding
&& ort != C_ORT_ACC to the while loop condition for CPP_OPEN_SQUARE?
(, . or * after id-expression is like any other unhandled characters...

>  	      while (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_SQUARE))
>  		{
>  		  tree low_bound = NULL_TREE, length = NULL_TREE;

> @@ -33868,7 +33885,7 @@ cp_parser_oacc_all_clauses (cp_parser *parser, omp_clause_mask mask,
>  	  c_name = "private";
>  	  break;
>  	case PRAGMA_OACC_CLAUSE_REDUCTION:
> -	  clauses = cp_parser_omp_clause_reduction (parser, clauses);
> +	  clauses = cp_parser_omp_clause_reduction (parser, clauses, C_ORT_ACC);
>  	  c_name = "reduction";
>  	  break;
>  	case PRAGMA_OACC_CLAUSE_SEQ:
> @@ -34055,7 +34072,7 @@ cp_parser_omp_all_clauses (cp_parser *parser, omp_clause_mask mask,
>  	  c_name = "private";
>  	  break;
>  	case PRAGMA_OMP_CLAUSE_REDUCTION:
> -	  clauses = cp_parser_omp_clause_reduction (parser, clauses);
> +	  clauses = cp_parser_omp_clause_reduction (parser, clauses, C_ORT_OMP);
>  	  c_name = "reduction";
>  	  break;
>  	case PRAGMA_OMP_CLAUSE_SCHEDULE:

Again, needs adjustement for IN_REDUCTION/TASK_REDUCTION.

> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> index c779137da45..177acdd9cc4 100644
> --- a/gcc/cp/semantics.c
> +++ b/gcc/cp/semantics.c
> @@ -5875,6 +5875,14 @@ finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
>  	  field_ok = ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP);
>  	  goto check_dup_generic;
>  	case OMP_CLAUSE_REDUCTION:
> +	  if (ort == C_ORT_ACC && oacc_get_fn_attrib (current_function_decl)
> +	      && omp_find_clause (clauses, OMP_CLAUSE_GANG))
> +	    {
> +	      error_at (OMP_CLAUSE_LOCATION (c),
> +			"gang reduction on an orphan loop");
> +	      remove = true;
> +	      break;
> +	    }
>  	  field_ok = ((ort & C_ORT_OMP_DECLARE_SIMD) == C_ORT_OMP);
>  	  t = OMP_CLAUSE_DECL (c);
>  	  if (TREE_CODE (t) == TREE_LIST)

In C++ finish_omp_clauses there are 2 loops, so you can easily just remember
if OMP_CLAUSE_GANG has been seen in the first loop and diagnose this in the
second loop only.

	Jakub