Re: [Patch] OpenMP: Parse align clause in allocate directive in C/C++

[Tobias Burnus <tobias@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 5166 bytes --]

Updated patch included. Changes:

* Removed xfail for C++

* For C, I updated the comment as suggested.

* For C++: I updated/extended the FIXME comment and added the 'align'
check (the simple version as first suggested; I did not went for the one
which supports some templates.)

Any further comments before I commit it?

Tobias

On 31.01.23 13:09, Jakub Jelinek wrote:
> On Tue, Dec 13, 2022 at 06:44:27PM +0100, Tobias Burnus wrote:
>> OpenMP: Parse align clause in allocate directive in C/C++
>>
>> gcc/c/ChangeLog:
>>
>>      * c-parser.cc (c_parser_omp_allocate): Parse align
>>      clause and check for restrictions.
>>
>> gcc/cp/ChangeLog:
>>
>>      * parser.cc (cp_parser_omp_allocate): Parse align
>>      clause.
>>
>> gcc/testsuite/ChangeLog:
>>
>>      * c-c++-common/gomp/allocate-5.c: Extend for align clause.
>>
>>   gcc/c/c-parser.cc                            | 88 ++++++++++++++++++++--------
>>   gcc/cp/parser.cc                             | 58 +++++++++++++-----
>>   gcc/testsuite/c-c++-common/gomp/allocate-5.c | 36 ++++++++++++
>>   3 files changed, 144 insertions(+), 38 deletions(-)
>>
>> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
>> index 1bbb39f9b08..62c302748dd 100644
>> --- a/gcc/c/c-parser.cc
>> +++ b/gcc/c/c-parser.cc
>> @@ -18819,32 +18819,71 @@ c_parser_oacc_wait (location_t loc, c_parser *parser, char *p_name)
>>     return stmt;
>>   }
>>
>> -/* OpenMP 5.0:
>> -   # pragma omp allocate (list)  [allocator(allocator)]  */
>> +/* OpenMP 5.x:
>> +   # pragma omp allocate (list)  clauses
>> +
>> +   OpenMP 5.0 clause:
>> +   allocator (omp_allocator_handle_t expression)
>> +
>> +   OpenMP 5.1 additional clause:
>> +   align (int expression)] */
> integer-expression or constant-expression or just expression.
> Also, 2 spaces before */ rather than just one.
>
>> +      else if (p[2] == 'i')
>> +    {
>> +      alignment = c_fully_fold (expr.value, false, NULL);
>> +      if (TREE_CODE (alignment) != INTEGER_CST
>> +          || !INTEGRAL_TYPE_P (TREE_TYPE (alignment))
>> +          || tree_int_cst_sgn (alignment) != 1
>> +          || !integer_pow2p (alignment))
> Note, the reason why we diagnose this in c-parser.cc and for C++
> in semantics.cc rather than in parser.cc are templates, it would be
> wasteful to handle it in two spots (parser.cc and during instantiation).
>
>> -  if (allocator)
>> +  if (allocator || alignment)
>>       for (tree c = nl; c != NULL_TREE; c = OMP_CLAUSE_CHAIN (c))
>> -      OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
>> +      {
>> +    OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
>> +    OMP_CLAUSE_ALLOCATE_ALIGN (c) = alignment;
>> +      }
> So, if you want for now until we actually support the directive
> properly diagnose it here in the parser (otherwise there is nothing
> to stick it on for later), then it would need either what semantics.cc
> currently uses:
>                if (!type_dependent_expression_p (align)
>                    && !INTEGRAL_TYPE_P (TREE_TYPE (align)))
>                  {
>                    error_at (OMP_CLAUSE_LOCATION (c),
>                              "%<allocate%> clause %<align%> modifier "
>                              "argument needs to be positive constant "
>                              "power of two integer expression");
>                    remove = true;
>                  }
>                else
>                  {
>                    align = mark_rvalue_use (align);
>                    if (!processing_template_decl)
>                      {
>                        align = maybe_constant_value (align);
>                        if (TREE_CODE (align) != INTEGER_CST
>                            || !tree_fits_uhwi_p (align)
>                            || !integer_pow2p (align))
>                          {
>                            error_at (OMP_CLAUSE_LOCATION (c),
>                                      "%<allocate%> clause %<align%> modifier "
>                                      "argument needs to be positive constant "
>                                      "power of two integer expression");
>                            remove = true;
>                          }
>                      }
>                  }
> with adjusted diagnostics, or perhaps instead of the
> !processing_template_decl guarding do
> fold_non_dependent_expr (align, !tf_none)
> instead of maybe_constant_value and just for
> processing_template_decl && TREE_CODE (align) != INTEGER_CST
> do nothing instead of the other tests and diagnostics.
> With the !processing_template_decl guarding it wouldn't diagnose
> ever non-power of two or non-constant operand in templates,
> with fold_non_dependent_expr etc. it would as long as they are
> not dependent.
>
> Otherwise LGTM, with or without the above changes.
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

[-- Attachment #2: align-c-v2.diff --]
[-- Type: text/x-patch, Size: 11930 bytes --]

OpenMP: Parse align clause in allocate directive in C/C++

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_omp_allocate): Parse align
	clause and check for restrictions.

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_omp_allocate): Parse align
	clause.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/allocate-5.c: Extend for align clause.

 gcc/c/c-parser.cc                            | 88 ++++++++++++++++++-------
 gcc/cp/parser.cc                             | 97 +++++++++++++++++++++++-----
 gcc/testsuite/c-c++-common/gomp/allocate-5.c | 36 +++++++++++
 3 files changed, 181 insertions(+), 40 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 69230002bc8..43427886ad4 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -18814,32 +18814,71 @@ c_parser_oacc_wait (location_t loc, c_parser *parser, char *p_name)
   return stmt;
 }
 
-/* OpenMP 5.0:
-   # pragma omp allocate (list)  [allocator(allocator)]  */
+/* OpenMP 5.x:
+   # pragma omp allocate (list)  clauses
+
+   OpenMP 5.0 clause:
+   allocator (omp_allocator_handle_t expression)
+
+   OpenMP 5.1 additional clause:
+   align (constant-expression)]  */
 
 static void
 c_parser_omp_allocate (location_t loc, c_parser *parser)
 {
+  tree alignment = NULL_TREE;
   tree allocator = NULL_TREE;
   tree nl = c_parser_omp_var_list_parens (parser, OMP_CLAUSE_ALLOCATE, NULL_TREE);
-  if (c_parser_next_token_is (parser, CPP_COMMA)
-      && c_parser_peek_2nd_token (parser)->type == CPP_NAME)
-    c_parser_consume_token (parser);
-  if (c_parser_next_token_is (parser, CPP_NAME))
+  do
     {
+      if (c_parser_next_token_is (parser, CPP_COMMA)
+	  && c_parser_peek_2nd_token (parser)->type == CPP_NAME)
+	c_parser_consume_token (parser);
+      if (!c_parser_next_token_is (parser, CPP_NAME))
+	break;
       matching_parens parens;
       const char *p = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
       c_parser_consume_token (parser);
-      if (strcmp ("allocator", p) != 0)
-	error_at (c_parser_peek_token (parser)->location,
-		  "expected %<allocator%>");
-      else if (parens.require_open (parser))
+      location_t expr_loc = c_parser_peek_token (parser)->location;
+      if (strcmp ("align", p) != 0 && strcmp ("allocator", p) != 0)
 	{
-	  location_t expr_loc = c_parser_peek_token (parser)->location;
-	  c_expr expr = c_parser_expr_no_commas (parser, NULL);
-	  expr = convert_lvalue_to_rvalue (expr_loc, expr, false, true);
-	  allocator = expr.value;
-	  allocator = c_fully_fold (allocator, false, NULL);
+	  error_at (c_parser_peek_token (parser)->location,
+		    "expected %<allocator%> or %<align%>");
+	  break;
+	}
+      if (!parens.require_open (parser))
+	break;
+
+      c_expr expr = c_parser_expr_no_commas (parser, NULL);
+      expr = convert_lvalue_to_rvalue (expr_loc, expr, false, true);
+      expr_loc = c_parser_peek_token (parser)->location;
+      if (p[2] == 'i' && alignment)
+	{
+	  error_at (expr_loc, "too many %qs clauses", "align");
+	  break;
+	}
+      else if (p[2] == 'i')
+	{
+	  alignment = c_fully_fold (expr.value, false, NULL);
+	  if (TREE_CODE (alignment) != INTEGER_CST
+	      || !INTEGRAL_TYPE_P (TREE_TYPE (alignment))
+	      || tree_int_cst_sgn (alignment) != 1
+	      || !integer_pow2p (alignment))
+	    {
+	      error_at (expr_loc, "%<align%> clause argument needs to be "
+				  "positive constant power of two integer "
+				  "expression");
+	      alignment = NULL_TREE;
+	    }
+	}
+      else if (allocator)
+	{
+	  error_at (expr_loc, "too many %qs clauses", "allocator");
+	  break;
+	}
+      else
+	{
+	  allocator = c_fully_fold (expr.value, false, NULL);
 	  tree orig_type
 	    = expr.original_type ? expr.original_type : TREE_TYPE (allocator);
 	  orig_type = TYPE_MAIN_VARIANT (orig_type);
@@ -18848,20 +18887,23 @@ c_parser_omp_allocate (location_t loc, c_parser *parser)
 	      || TYPE_NAME (orig_type)
 		 != get_identifier ("omp_allocator_handle_t"))
 	    {
-	      error_at (expr_loc, "%<allocator%> clause allocator expression "
-				"has type %qT rather than "
-				"%<omp_allocator_handle_t%>",
-				TREE_TYPE (allocator));
+	      error_at (expr_loc,
+			"%<allocator%> clause allocator expression has type "
+			"%qT rather than %<omp_allocator_handle_t%>",
+			TREE_TYPE (allocator));
 	      allocator = NULL_TREE;
 	    }
-	  parens.skip_until_found_close (parser);
 	}
-    }
+      parens.skip_until_found_close (parser);
+    } while (true);
   c_parser_skip_to_pragma_eol (parser);
 
-  if (allocator)
+  if (allocator || alignment)
     for (tree c = nl; c != NULL_TREE; c = OMP_CLAUSE_CHAIN (c))
-      OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+      {
+	OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+	OMP_CLAUSE_ALLOCATE_ALIGN (c) = alignment;
+      }
 
   sorry_at (loc, "%<#pragma omp allocate%> not yet supported");
 }
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 4cdc1cd472f..a89ab284a38 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -41483,43 +41483,106 @@ cp_parser_omp_structured_block (cp_parser *parser, bool *if_p)
   return finish_omp_structured_block (stmt);
 }
 
-/* OpenMP 5.0:
-   # pragma omp allocate (list)  [allocator(allocator)]  */
+/* OpenMP 5.x:
+   # pragma omp allocate (list)  clauses
+
+   OpenMP 5.0 clause:
+   allocator (omp_allocator_handle_t expression)
+
+   OpenMP 5.1 additional clause:
+   align (constant-expression)]  */
 
 static void
 cp_parser_omp_allocate (cp_parser *parser, cp_token *pragma_tok)
 {
   tree allocator = NULL_TREE;
+  tree alignment = NULL_TREE;
   location_t loc = pragma_tok->location;
   tree nl = cp_parser_omp_var_list (parser, OMP_CLAUSE_ALLOCATE, NULL_TREE);
 
-  if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA)
-      && cp_lexer_nth_token_is (parser->lexer, 2, CPP_NAME))
-    cp_lexer_consume_token (parser->lexer);
-
-  if (cp_lexer_next_token_is (parser->lexer, CPP_NAME))
+  do
     {
+      if (cp_lexer_next_token_is (parser->lexer, CPP_COMMA)
+	  && cp_lexer_nth_token_is (parser->lexer, 2, CPP_NAME))
+	cp_lexer_consume_token (parser->lexer);
+
+      if (!cp_lexer_next_token_is (parser->lexer, CPP_NAME))
+	break;
       matching_parens parens;
       tree id = cp_lexer_peek_token (parser->lexer)->u.value;
       const char *p = IDENTIFIER_POINTER (id);
       location_t cloc = cp_lexer_peek_token (parser->lexer)->location;
       cp_lexer_consume_token (parser->lexer);
-      if (strcmp (p, "allocator") != 0)
-	error_at (cloc, "expected %<allocator%>");
-      else if (parens.require_open (parser))
+      if (strcmp (p, "allocator") != 0 && strcmp (p, "align") != 0)
 	{
-	  allocator = cp_parser_assignment_expression (parser);
-	  if (allocator == error_mark_node)
-	    allocator = NULL_TREE;
-	  parens.require_close (parser);
+	  error_at (cloc, "expected %<allocator%> or %<align%>");
+	  break;
 	}
-    }
+      if (!parens.require_open (parser))
+	break;
+      tree expr = cp_parser_assignment_expression (parser);
+      if (p[2] == 'i' && alignment)
+	{
+	  error_at (cloc, "too many %qs clauses", "align");
+	  break;
+	}
+      else if (p[2] == 'i')
+	{
+	  if (expr != error_mark_node)
+	    alignment = expr;
+	  /* FIXME: Remove when adding check to semantic.cc; cf FIXME below.  */
+	  if (alignment
+	      && !type_dependent_expression_p (alignment)
+	      && !INTEGRAL_TYPE_P (TREE_TYPE (alignment)))
+	    {
+              error_at (cloc, "%<align%> clause argument needs to be "
+			      "positive constant power of two integer "
+			      "expression");
+	      alignment = NULL_TREE;
+	    }
+	  else if (alignment)
+	    {
+	      alignment = mark_rvalue_use (alignment);
+	      if (!processing_template_decl)
+		{
+		  alignment = maybe_constant_value (alignment);
+		  if (TREE_CODE (alignment) != INTEGER_CST
+		      || !tree_fits_uhwi_p (alignment)
+		      || !integer_pow2p (alignment))
+		    {
+		      error_at (cloc, "%<align%> clause argument needs to be "
+				      "positive constant power of two integer "
+				      "expression");
+		      alignment = NULL_TREE;
+		    }
+		}
+	    }
+	}
+      else if (allocator)
+	{
+	  error_at (cloc, "too many %qs clauses", "allocator");
+	  break;
+	}
+      else
+	{
+	  if (expr != error_mark_node)
+	    allocator = expr;
+	}
+      parens.require_close (parser);
+    } while (true);
   cp_parser_require_pragma_eol (parser, pragma_tok);
 
-  if (allocator)
+  if (allocator || alignment)
     for (tree c = nl; c != NULL_TREE; c = OMP_CLAUSE_CHAIN (c))
-      OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+      {
+	OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+	OMP_CLAUSE_ALLOCATE_ALIGN (c) = alignment;
+      }
 
+  /* FIXME: When implementing properly, delete the align/allocate expr error
+     check above and add one in semantics.cc (to properly handle templates).
+     Base this on the allocator/align modifiers check for the 'allocate' clause
+     in semantics.cc's finish_omp_clauses.  */
   sorry_at (loc, "%<#pragma omp allocate%> not yet supported");
 }
 
diff --git a/gcc/testsuite/c-c++-common/gomp/allocate-5.c b/gcc/testsuite/c-c++-common/gomp/allocate-5.c
index 34dcb48c3d7..8a9181205f7 100644
--- a/gcc/testsuite/c-c++-common/gomp/allocate-5.c
+++ b/gcc/testsuite/c-c++-common/gomp/allocate-5.c
@@ -39,3 +39,39 @@ bar ()
 #pragma omp allocate(a) allocator(b)  /* { dg-error "'allocator' clause allocator expression has type 'int' rather than 'omp_allocator_handle_t'" "todo: cp/semantics.c" { xfail c++ } } */
   /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
 }
+
+
+void
+align_test ()
+{
+  int i;
+  #pragma omp allocate(i) allocator(omp_default_mem_alloc), align(32)
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+  #pragma omp allocate(i) align ( 32 ),allocator(omp_default_mem_alloc)
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+  #pragma omp allocate(i),allocator(omp_default_mem_alloc) align(32)
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+  #pragma omp allocate(i) align ( 32 ) allocator(omp_default_mem_alloc)
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+
+  #pragma omp allocate(i) allocator ( omp_high_bw_mem_alloc ), align ( 32 ) allocator(omp_default_mem_alloc)
+  /* { dg-error "too many 'allocator' clauses" "" { target *-*-* } .-1 } */
+  /* { dg-error "expected end of line before '\\)' token" "" { target *-*-* } .-2 } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-3 } */
+  #pragma omp allocate(i) align ( 32 ), align(32) allocator(omp_default_mem_alloc)
+  /* { dg-error "too many 'align' clauses" "" { target *-*-* } .-1 } */
+  /* { dg-error "expected end of line before '\\)' token" "" { target *-*-* } .-2 } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-3 } */
+}
+
+void
+align_test2 ()
+{
+  int i;
+  #pragma omp allocate(i) align (32.0)  /* { dg-error "'align' clause argument needs to be positive constant power of two integer expression" } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+  #pragma omp allocate(i) align ( 31 )  /* { dg-error "'align' clause argument needs to be positive constant power of two integer expression" } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+  #pragma omp allocate(i) align ( -32 )  /* { dg-error "'align' clause argument needs to be positive constant power of two integer expression" } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+}