diff mbox series

OpenMP: C/C++ parse 'omp allocate'

Message ID bb0f6ce0-f0c1-0ea1-855a-7f9fd4f2563c@codesourcery.com
State New
Headers show
Series OpenMP: C/C++ parse 'omp allocate' | expand

Commit Message

Tobias Burnus Nov. 23, 2020, 2:50 p.m. UTC 
Given that (at least for C/C++) there is some initial support for
OpenMP 5.0's allocators, it is likely that users will try it.
Also the release notes state: "the allocator routines of OpenMP 5.0,
including initial|allocate|  clause support in C/C++."

The latter does not include the omp allocate directive, still,
it can be expected that users will try:

   #pragma omp allocate(...)

And that will fail at runtime. I think that's undesirable,
even if - like any unknown directive - -Wunknown-pragmas
(-Wall) warns about it.

Thoughts? OK?

Tobias

PS: I have not tried to implement restrictions or additions
like 'allocate(a[5])', which is currently rejected. I also
did not check whether there are differences between the clause
([partially] implemented) and the directive (this patch).

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Comments

Jakub Jelinek Dec. 8, 2020, 5:56 p.m. UTC | #1 
On Mon, Nov 23, 2020 at 03:50:33PM +0100, Tobias Burnus wrote:
> Given that (at least for C/C++) there is some initial support for
> OpenMP 5.0's allocators, it is likely that users will try it.

Sadly at least the current implementation doesn't offer much benefits;
I meant to add e.g. HBM support through dlopening of the memkind library,
but I haven't found a box with hw I could test it on.
Something that could be done is the memory pinning, we can use mlock for
that at least on Linux, the question is how to handle small allocations.
Larger allocations (say 2 or more pages) we could just round to number of
pages with page alignment and mlock that part and munlock on freeing.
Also, we should do something smarter for the NVPTX and AMDGCN offloading
targets for the allocators, perhaps handle omp_alloc etc. with some constant
allocator arguments directly through PTX etc. directives.

> Also the release notes state: "the allocator routines of OpenMP 5.0,
> including initial|allocate|  clause support in C/C++."
> 
> The latter does not include the omp allocate directive, still,
> it can be expected that users will try:
> 
>   #pragma omp allocate(...)
> 
> And that will fail at runtime. I think that's undesirable,
> even if - like any unknown directive - -Wunknown-pragmas
> (-Wall) warns about it.
> 
> Thoughts? OK?
> 
> Tobias
> 
> PS: I have not tried to implement restrictions or additions
> like 'allocate(a[5])', which is currently rejected. I also

I think allocate(a[5]) is not valid, allocate can't mention parts of other
variables (array elements, array sections, structure members).

> did not check whether there are differences between the clause
> ([partially] implemented) and the directive (this patch).

I guess your patch is ok, but I should fine time to implement at least
the rest of the restrictions; in particular e.g.:

A declarative allocate directive must appear in the same scope as the declarations of each of
its list items and must follow all such declarations.

Check if the current scope is the scope that contains all the vars.

Stick the allocator as an artificial attribute to the decls rather than
throwing it away.

I think we should implement also the 5.1 restriction:
A declared variable may appear as a list item in at most one declarative allocate directive in a
given compilation unit.
because having multiple allocate directives for the same variable is just
insane and unspecified what it would do.

While the patch tests for C that the allocator has the right type, for C++
(for obvious reasons) it isn't checked, so we need the checking there later
from the attributes or so, at least if it is dependent.

For static storage vars, we need to verify the allocator is a constant
expression, and most likely otherwise just ignore, unless we want to use say
PTX etc. directives to allocate stuff in special kinds of memory.

For automatic variables, we likely need to handle it during gimplification,
that is the last time we can reasonably add the destructors easily
(GOMP_free) such that it would be destructed on C++ exceptions, goto out of
scope etc.

> OpenMP: C/C++ parse 'omp allocate'
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-pragma.c (omp_pragmas): Add 'allocate'.
> 	* c-pragma.h (enum pragma_kind): Add PRAGMA_OMP_ALLOCATE.
> 
> gcc/c/ChangeLog:
> 
> 	* c-parser.c (c_parser_omp_allocate): New.
> 	(c_parser_omp_construct): Call it.
> 
> gcc/cp/ChangeLog:
> 
> 	* parser.c (cp_parser_omp_allocate): New.
> 	(cp_parser_omp_construct, cp_parser_pragma): Call it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* c-c++-common/gomp/allocate-5.c: New test.

Ok, thanks.

	Jakub
Tobias Burnus Dec. 9, 2020, 11:20 a.m. UTC | #2 
On 08.12.20 18:56, Jakub Jelinek wrote:
> On Mon, Nov 23, 2020 at 03:50:33PM +0100, Tobias Burnus wrote:
>> Given that (at least for C/C++) there is some initial support for
>> OpenMP 5.0's allocators, it is likely that users will try it.
> Sadly at least the current implementation doesn't offer much benefits;
> I meant to add e.g. HBM support through dlopening of the memkind library,
> but I haven't found a box with hw I could test it on.

I am not sure that there is a big benefit for most of the allocation
options – well, maybe in theory or for cluster OpenMP solutions with
slow interconnect or maybe for those SGI Altrix systems with lots of
threads.

The largest effect should be there for offloading and unified-shared
memory systems, where pinning seems to be essential for good performance.

> I guess your patch is ok, but I should fine time to implement at least
> the rest of the restrictions; in particular e.g.:
> ...
> While the patch tests for C that the allocator has the right type, for C++
> (for obvious reasons) it isn't checked, so we need the checking there later
> from the attributes or so, at least if it is dependent.

For 'allocate' attribute, it is checked in cp/semantics.c's
finish_omp_clauses; I think it could be done there as well. – Once, the
'sorry' is either removed or moved to cp/semantics.c.

> For automatic variables, we likely need to handle it during gimplification,
That's outside of this patch, but I fear what we will need to do for
Fortran with allocatable components.
> Ok, thanks

Thanks for the review. Now committed as
r11-5879-gaa0432005f36f6ac51dc9dcecb717fe739d39b88.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
diff mbox series

Patch

OpenMP: C/C++ parse 'omp allocate'

gcc/c-family/ChangeLog:

	* c-pragma.c (omp_pragmas): Add 'allocate'.
	* c-pragma.h (enum pragma_kind): Add PRAGMA_OMP_ALLOCATE.

gcc/c/ChangeLog:

	* c-parser.c (c_parser_omp_allocate): New.
	(c_parser_omp_construct): Call it.

gcc/cp/ChangeLog:

	* parser.c (cp_parser_omp_allocate): New.
	(cp_parser_omp_construct, cp_parser_pragma): Call it.

gcc/testsuite/ChangeLog:

	* c-c++-common/gomp/allocate-5.c: New test.

 gcc/c-family/c-pragma.c                      |  1 +
 gcc/c-family/c-pragma.h                      |  1 +
 gcc/c/c-parser.c                             | 52 ++++++++++++++++++++++++++++
 gcc/cp/parser.c                              | 43 ++++++++++++++++++++++-
 gcc/testsuite/c-c++-common/gomp/allocate-5.c | 41 ++++++++++++++++++++++
 5 files changed, 137 insertions(+), 1 deletion(-)

diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index d68985ca277..e80dcd7c0a1 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1309,6 +1309,7 @@  static const struct omp_pragma_def oacc_pragmas[] = {
   { "wait", PRAGMA_OACC_WAIT }
 };
 static const struct omp_pragma_def omp_pragmas[] = {
+  { "allocate", PRAGMA_OMP_ALLOCATE },
   { "atomic", PRAGMA_OMP_ATOMIC },
   { "barrier", PRAGMA_OMP_BARRIER },
   { "cancel", PRAGMA_OMP_CANCEL },
diff --git a/gcc/c-family/c-pragma.h b/gcc/c-family/c-pragma.h
index 5a493fe5175..e0e4da6b6b3 100644
--- a/gcc/c-family/c-pragma.h
+++ b/gcc/c-family/c-pragma.h
@@ -42,6 +42,7 @@  enum pragma_kind {
   PRAGMA_OACC_UPDATE,
   PRAGMA_OACC_WAIT,
 
+  PRAGMA_OMP_ALLOCATE,
   PRAGMA_OMP_ATOMIC,
   PRAGMA_OMP_BARRIER,
   PRAGMA_OMP_CANCEL,
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 7540a15d65d..fddd5c3c8c6 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -17255,6 +17255,55 @@  c_parser_oacc_wait (location_t loc, c_parser *parser, char *p_name)
   return stmt;
 }
 
+/* OpenMP 5.0:
+   # pragma omp allocate (list)  [allocator(allocator)]  */
+
+static void
+c_parser_omp_allocate (location_t loc, c_parser *parser)
+{
+  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_NAME))
+    {
+      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;
+	  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);
+	  tree orig_type
+	    = expr.original_type ? expr.original_type : TREE_TYPE (allocator);
+	  orig_type = TYPE_MAIN_VARIANT (orig_type);
+	  if (!INTEGRAL_TYPE_P (TREE_TYPE (allocator))
+	      || TREE_CODE (orig_type) != ENUMERAL_TYPE
+	      || 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));
+	      allocator = NULL_TREE;
+	    }
+	  parens.skip_until_found_close (parser);
+	}
+    }
+  c_parser_skip_to_pragma_eol (parser);
+
+  if (allocator)
+    for (tree c = nl; c != NULL_TREE; c = OMP_CLAUSE_CHAIN (c))
+      OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+
+  sorry_at (loc, "%<#pragma omp allocate%> not yet supported");
+}
+
 /* OpenMP 2.5:
    # pragma omp atomic new-line
      expression-stmt
@@ -21542,6 +21591,9 @@  c_parser_omp_construct (c_parser *parser, bool *if_p)
       strcpy (p_name, "#pragma wait");
       stmt = c_parser_oacc_wait (loc, parser, p_name);
       break;
+    case PRAGMA_OMP_ALLOCATE:
+      c_parser_omp_allocate (loc, parser);
+      return;
     case PRAGMA_OMP_ATOMIC:
       c_parser_omp_atomic (loc, parser, false);
       return;
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 88021243ee4..54e71e9743b 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -38136,6 +38136,42 @@  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)]  */
+
+static void
+cp_parser_omp_allocate (cp_parser *parser, cp_token *pragma_tok)
+{
+  tree allocator = 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_NAME))
+    {
+      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))
+	{
+	  allocator = cp_parser_assignment_expression (parser);
+	  if (allocator == error_mark_node)
+	    allocator = NULL_TREE;
+	  parens.require_close (parser);
+	}
+    }
+  cp_parser_require_pragma_eol (parser, pragma_tok);
+
+  if (allocator)
+    for (tree c = nl; c != NULL_TREE; c = OMP_CLAUSE_CHAIN (c))
+      OMP_CLAUSE_ALLOCATE_ALLOCATOR (c) = allocator;
+
+  sorry_at (loc, "%<#pragma omp allocate%> not yet supported");
+}
+
 /* OpenMP 2.5:
    # pragma omp atomic new-line
      expression-stmt
@@ -43767,6 +43803,9 @@  cp_parser_omp_construct (cp_parser *parser, cp_token *pragma_tok, bool *if_p)
     case PRAGMA_OACC_WAIT:
       stmt = cp_parser_oacc_wait (parser, pragma_tok);
       break;
+    case PRAGMA_OMP_ALLOCATE:
+      cp_parser_omp_allocate (parser, pragma_tok);
+      return;
     case PRAGMA_OMP_ATOMIC:
       cp_parser_omp_atomic (parser, pragma_tok, false);
       return;
@@ -44412,7 +44451,9 @@  cp_parser_pragma (cp_parser *parser, enum pragma_context context, bool *if_p)
 	goto bad_stmt;
       cp_parser_omp_construct (parser, pragma_tok, if_p);
       return true;
-
+    case PRAGMA_OMP_ALLOCATE:
+      cp_parser_omp_allocate (parser, pragma_tok);
+      return false;
     case PRAGMA_OACC_ATOMIC:
     case PRAGMA_OACC_CACHE:
     case PRAGMA_OACC_DATA:
diff --git a/gcc/testsuite/c-c++-common/gomp/allocate-5.c b/gcc/testsuite/c-c++-common/gomp/allocate-5.c
new file mode 100644
index 00000000000..34dcb48c3d7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/gomp/allocate-5.c
@@ -0,0 +1,41 @@ 
+typedef enum omp_allocator_handle_t
+#if __cplusplus >= 201103L
+: __UINTPTR_TYPE__
+#endif
+{
+  omp_null_allocator = 0,
+  omp_default_mem_alloc = 1,
+  omp_large_cap_mem_alloc = 2,
+  omp_const_mem_alloc = 3,
+  omp_high_bw_mem_alloc = 4,
+  omp_low_lat_mem_alloc = 5,
+  omp_cgroup_mem_alloc = 6,
+  omp_pteam_mem_alloc = 7,
+  omp_thread_mem_alloc = 8,
+  __omp_allocator_handle_t_max__ = __UINTPTR_MAX__
+} omp_allocator_handle_t;
+
+void
+foo ()
+{
+  int a, b;
+  omp_allocator_handle_t my_allocator;
+#pragma omp allocate (a)  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" } */
+#pragma omp allocate (b) allocator(my_allocator)  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" } */
+}
+
+void
+bar ()
+{
+  int a, b;
+  omp_allocator_handle_t my_allocator;
+#pragma omp allocate  /* { dg-error "expected '\\(' before end of line" } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+#pragma omp allocate allocator(my_allocator)  /* { dg-error "expected '\\(' before 'allocator'" } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-1 } */
+#pragma omp allocate(a) foo(my_allocator) /* { dg-error "expected 'allocator'" } */
+  /* { dg-error "expected end of line before '\\(' token" "" { target *-*-* } .-1 } */
+  /* { dg-message "sorry, unimplemented: '#pragma omp allocate' not yet supported" "" { target *-*-* } .-2 } */
+#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 } */
+}