diff mbox series

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

Message ID 16bc2d0c-7228-43f8-803b-74a980510370@codesourcery.com
State New
Headers show
Series OpenMP: Minor '!$omp allocators' cleanup - and still: Re: [patch] OpenMP/Fortran: Implement omp allocators/allocate for ptr/allocatables | expand

Commit Message

Tobias Burnus Dec. 11, 2023, 11:45 a.m. UTC 
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

Comments

Thomas Schwinge Dec. 11, 2023, 10:12 p.m. UTC | #1 
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
<https://inbox.sourceware.org/1acd7994-2440-4092-897f-97f14d3fbf45@redhat.com>
"[TTYPE] Strongly typed tree project. Original document circa 2017".)

On 2023-12-11T12:45:27+0100, Tobias Burnus <tobias@codesourcery.com> 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
Andrew MacLeod Dec. 11, 2023, 11:45 p.m. UTC | #2 
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
> <https://inbox.sourceware.org/1acd7994-2440-4092-897f-97f14d3fbf45@redhat.com>
> "[TTYPE] Strongly typed tree project. Original document circa 2017".)
>
> On 2023-12-11T12:45:27+0100, Tobias Burnus <tobias@codesourcery.com> 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
diff mbox series

Patch

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", 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);
 }
diff --git a/libgomp/libgomp_g.h b/libgomp/libgomp_g.h
index 95046312ae9..ec619f255f2 100644
--- a/libgomp/libgomp_g.h
+++ b/libgomp/libgomp_g.h
@@ -366,6 +366,9 @@  extern void GOMP_teams_reg (void (*) (void *), void *, unsigned, unsigned,
 
 /* allocator.c */
 
+extern void GOMP_add_alloc (void *);
+extern bool GOMP_is_alloc (void *);
+
 extern void *GOMP_alloc (size_t, size_t, uintptr_t);
 extern void GOMP_free (void *, uintptr_t);