diff mbox series

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

Message ID 857e44cb-92ce-7f1d-c036-579d2e345107@codesourcery.com
State New
Headers show
Series OpenMP: Parse align clause in allocate directive in C/C++ | expand

Commit Message

Tobias Burnus Dec. 13, 2022, 5:44 p.m. UTC 
We have a working parsing support for the 'allocate' directive
(failing immediately with a sorry after parsing).

To be in line with the rest of the allocat(e,or) etc. handling,
it makes sense to take care of 'align' as well, which is this
patch does - it still fails with a 'sorry' after parsing.

OK for mainline?

Tobias
-----------------
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

Comments

Jakub Jelinek Jan. 31, 2023, 12:09 p.m. UTC | #1 
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.

	Jakub
Tobias Burnus Feb. 9, 2023, 10:16 a.m. UTC | #2 
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
Jakub Jelinek Feb. 9, 2023, 10:25 a.m. UTC | #3 
On Thu, Feb 09, 2023 at 11:16:39AM +0100, Tobias Burnus wrote:
> Any further comments before I commit it?
> 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.

The " and check for restrictions" part now applies also to C++...

> +	  if (expr != error_mark_node)
> +	    alignment = expr;
> +	  /* FIXME: Remove when adding check to semantic.cc; cf FIXME below.  */

s/semantic.cc/semantics.cc/

Ok with those nits fixed.

	Jakub
diff mbox series

Patch

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)] */
 
 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);
@@ -18853,20 +18892,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 4798aae1fbb..d284022266f 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -41398,36 +41398,64 @@  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;
+	}
+      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;
+      }
 
+  /* align/allocator needs same check as the modifiers to the
+     'allocator' 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..997e7f06896 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" "todo: cp/semantics.c" { xfail c++ } } */
+  /* { 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" "todo: cp/semantics.c" { xfail c++ } } */
+  /* { 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" "todo: cp/semantics.c" { xfail c++ } } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+}