Re: [Patch] OpenMP: Minor '!$omp allocators' cleanup - and still: Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables

[Andrew MacLeod]

On 12/11/23 17:12, Thomas Schwinge wrote:

Hi!

This issue would've been prevented if we'd actually use a distinct C++
data type for GCC types, checkable at compile time -- I'm thus CCing
Andrew MacLeod for amusement or crying, "one more for the list!".  ;-\


Perhaps the time has come  It is definitely under re-consideration 
for next stage 1...


Andrew


(See

"[TTYPE] Strongly typed tree project. Original document circa 2017".)

On 2023-12-11T12:45:27+0100, Tobias Burnus  wrote:

I included a minor cleanup patch [...]

I intent to commit that patch as obvious, unless there are further comments.
OpenMP: Minor '!$omp allocators' cleanup
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -8361,8 +8361,10 @@ gfc_omp_call_add_alloc (tree ptr)
if (fn == NULL_TREE)
  {
fn = build_function_type_list (void_type_node, ptr_type_node, 
NULL_TREE);
+  tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
+  att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES (fn));
+  fn = build_type_attribute_variant (fn, att);
fn = build_fn_decl ("GOMP_add_alloc", fn);
-/* FIXME: attributes.  */
  }
return build_call_expr_loc (input_location, fn, 1, ptr);
  }
@@ -8380,7 +8382,9 @@ gfc_omp_call_is_alloc (tree ptr)
fn = build_function_type_list (boolean_type_node, ptr_type_node,
NULL_TREE);
fn = build_fn_decl ("GOMP_is_alloc", fn);
-/* FIXME: attributes.  */
+  tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
+  att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES (fn));
+  fn = build_type_attribute_variant (fn, att);
  }
return build_call_expr_loc (input_location, fn, 1, ptr);
  }

Pushed to master branch commit 453e0f45a49f425992bc47ff8909ed8affc29d2e
"Resolve ICE in 'gcc/fortran/trans-openmp.cc:gfc_omp_call_is_alloc'", see
attached.


Grüße
  Thomas


-
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

Re: [Patch] OpenMP: Minor '!$omp allocators' cleanup - and still: Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables

[Thomas Schwinge]

Hi!

This issue would've been prevented if we'd actually use a distinct C++
data type for GCC types, checkable at compile time -- I'm thus CCing
Andrew MacLeod for amusement or crying, "one more for the list!".  ;-\
(See

"[TTYPE] Strongly typed tree project. Original document circa 2017".)

On 2023-12-11T12:45:27+0100, Tobias Burnus  wrote:
> I included a minor cleanup patch [...]
>
> I intent to commit that patch as obvious, unless there are further comments.

> OpenMP: Minor '!$omp allocators' cleanup

> --- a/gcc/fortran/trans-openmp.cc
> +++ b/gcc/fortran/trans-openmp.cc
> @@ -8361,8 +8361,10 @@ gfc_omp_call_add_alloc (tree ptr)
>if (fn == NULL_TREE)
>  {
>fn = build_function_type_list (void_type_node, ptr_type_node, 
> NULL_TREE);
> +  tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
> +  att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES 
> (fn));
> +  fn = build_type_attribute_variant (fn, att);
>fn = build_fn_decl ("GOMP_add_alloc", fn);
> -/* FIXME: attributes.  */
>  }
>return build_call_expr_loc (input_location, fn, 1, ptr);
>  }
> @@ -8380,7 +8382,9 @@ gfc_omp_call_is_alloc (tree ptr)
>fn = build_function_type_list (boolean_type_node, ptr_type_node,
>NULL_TREE);
>fn = build_fn_decl ("GOMP_is_alloc", fn);
> -/* FIXME: attributes.  */
> +  tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
> +  att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES 
> (fn));
> +  fn = build_type_attribute_variant (fn, att);
>  }
>return build_call_expr_loc (input_location, fn, 1, ptr);
>  }

Pushed to master branch commit 453e0f45a49f425992bc47ff8909ed8affc29d2e
"Resolve ICE in 'gcc/fortran/trans-openmp.cc:gfc_omp_call_is_alloc'", see
attached.


Grüße
 Thomas


-
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
>From 453e0f45a49f425992bc47ff8909ed8affc29d2e Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Mon, 11 Dec 2023 22:52:54 +0100
Subject: [PATCH] Resolve ICE in
 'gcc/fortran/trans-openmp.cc:gfc_omp_call_is_alloc'

Fix-up for recent commit 2505a8b41d3b74a545755a278f3750a29c1340b6
"OpenMP: Minor '!$omp allocators' cleanup", which caused:

{+FAIL: gfortran.dg/gomp/allocate-5.f90   -O  (internal compiler error: tree check: expected class 'type', have 'declaration' (function_decl) in gfc_omp_call_is_alloc, at fortran/trans-openmp.cc:8386)+}
[-PASS:-]{+FAIL:+} gfortran.dg/gomp/allocate-5.f90   -O  (test for excess errors)

..., and similarly in 'libgomp.fortran/allocators-1.f90',
'libgomp.fortran/allocators-2.f90', 'libgomp.fortran/allocators-3.f90',
'libgomp.fortran/allocators-4.f90', 'libgomp.fortran/allocators-5.f90'.

	gcc/fortran/
	* trans-openmp.cc (gfc_omp_call_is_alloc): Resolve ICE.
---
 gcc/fortran/trans-openmp.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 95184920cf7..f7c73a5d273 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -8381,10 +8381,10 @@ gfc_omp_call_is_alloc (tree ptr)
 {
   fn = build_function_type_list (boolean_type_node, ptr_type_node,
  NULL_TREE);
-  fn = build_fn_decl ("GOMP_is_alloc", fn);
   tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
   att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES (fn));
   fn = build_type_attribute_variant (fn, att);
+  fn = build_fn_decl ("GOMP_is_alloc", fn);
 }
   return build_call_expr_loc (input_location, fn, 1, ptr);
 }
-- 
2.34.1

[Patch] OpenMP: Minor '!$omp allocators' cleanup - and still: Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables

[Tobias Burnus]

Hi Thomas & Jakub,

I included a minor cleanup patch - but the rest is really a bunch of RFC.

I intent to commit that patch as obvious, unless there are further comments.

On 09.12.23 16:14, Jakub Jelinek wrote:


There were 3 reasons to add GOMP_alloc (and 1 for GOMP_free):
1) when it was added, omp_aligned_alloc was still not exported from the
library because we thought we shouldn't expose 5.1 features until we
have 5.0 implemented (then changed mind)
2) unline omp_aligned_alloc, GOMP_alloc issues fatal error on allocation
failure
3) the omp_* functions have omp_allocator_handle_t arguments, which is hard
to provide for builtins (I think this is the only reason for GOMP_free
addition, maybe together with wanting those to be paired)


Is this a real issue? GOMP_{alloc,free} take uintptr_t as allocator
while omp_* take omp_allocator_handle_t.

But that contains a __omp_memspace_handle_t_max__ = __UINTPTR_MAX__ and
>= C++ 11 ': __UINTPTR_TYPE__' for C/C++ and omp_allocator_handle_kind
= c_intptr_t in Fortran (→ integer(c_intptr_t) = signed variant of
uintptr_t).

In Fortran, there is an explicit check that the allocator has that kind,
which is a check whether it is kind=4 or kind=8, respectively, depending
what kind matches intptr_t. Thus, for Fortran, there is already a mismatch.

Thus, it seems to be at most a C/C++ issue.


Now, 1) is a non-issue anymore, I don't know what Fortran wants for
allocation failures, if it is better to have diagnostics on the libgomp side
or if wants to emit it inline.


I think that's not quite clear, while for Fortran itself and for
OpenMP's omp_alloc routine, the behavior is well defined, it is not for
'!$omp allocators'.

However, I think using omp_alloc is more Fortranish.

I think it would be a tad cleaner to change it – but the question is how
to do it best:

Even when ignoring the uintptr_t vs. omp_allocator_handle_t issue, the
question is still how to handle it best.

Create another builtin - to make it possible to add it to
gimple-ssa-warn-access.cc / tree-ssa-ccp.cc / tree-ssa-dce.cc?

Or the alternative: Be more un-Fortranish and keep the current
GOMP_alloc handling?

Thoughts?


And yes, 3) would be an argument to add
GOMP_realloc.


I am happy to add GOMP_realloc – passing 0 for old/new allocator in case
of Fortran - if it really makes sense. Otherwise, I'd keep it as is.

I think the next user would be the (pseudo-)USM patches, i.e. replacing
all (re,c,m)alloc/free by calls to omp_(re,c,)alloc/omp_free + special
allocator when -foffload-memory={none,pinned,unified} (well, not when
=none). Those are a bit fragile but permit using pinned/USM on less
capable offload devices. (Feature exists also with other compilers, i.e.
the users seem to be used to the limitations of this feature.)

Thus, one option would be to wait until that feature is ready.

What would be the NULL behavior? As omp_alloc or as GOMP_alloc? I assume
as omp_alloc would be fine?

* * *

On 09.12.23 12:19, Thomas Schwinge wrote:

Why not define 'GOMP_add_alloc', 'GOMP_is_alloc' via 'gcc/omp-builtins.def'?


We could - but does it make sense to have it for a single caller?


Should this either be 'BUILT_IN_OMP_REALLOC' ('OMP' instead of 'GOMP'),
or otherwise a 'GOMP_realloc' be added to 'libgomp/allocator.c

Cf. also above. but I am fine with a name change to OMP_ as well/in addition.


'GOMP_add_alloc', 'GOMP_is_alloc' should get prototyped in
'libgomp/libgomp_g.h'.


I concur + added.

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
OpenMP: Minor '!$omp allocators' cleanup

gcc/fortran/ChangeLog:

	* trans-openmp.cc (gfc_omp_call_add_alloc,
	gfc_omp_call_is_alloc): Set 'fn spec'.

libgomp/ChangeLog:

	* libgomp_g.h (GOMP_add_alloc, GOMP_is_alloc): Add.

 gcc/fortran/trans-openmp.cc | 8 ++--
 libgomp/libgomp_g.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gcc/fortran/trans-openmp.cc b/gcc/fortran/trans-openmp.cc
index 9e166c94f8e..95184920cf7 100644
--- a/gcc/fortran/trans-openmp.cc
+++ b/gcc/fortran/trans-openmp.cc
@@ -8361,8 +8361,10 @@ gfc_omp_call_add_alloc (tree ptr)
   if (fn == NULL_TREE)
 {
   fn = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
+  tree att = build_tree_list (NULL_TREE, build_string (4, ". R "));
+  att = tree_cons (get_identifier ("fn spec"), att, TYPE_ATTRIBUTES (fn));
+  fn = build_type_attribute_variant (fn, att);
   fn = build_fn_decl ("GOMP_add_alloc", fn);
-/* FIXME: attributes.  */
 }
   return build_call_expr_loc (input_location, fn, 1, ptr);
 }
@@ -8380,7 +8382,9 @@ gfc_omp_call_is_alloc (tree ptr)
   fn = build_function_type_list (boolean_type_node, ptr_type_node,
  NULL_TREE);
   fn = build_fn_decl ("GOMP_is_alloc",