[Patch,v4] Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings
[GCC 13 vs GCC 14] I am unsure whether this should still go to GCC 13 or not. It is somewhat larger albeit well contained (Fortran, only OpenMP, ...) and fixes real-world bugs, but it is not a regression - and we are meanwhile slowly approaching the release. An alternative would be to go to GCC 14 and then be eventually backported to GCC 14 (albeit I am not sure whether early testing would be better). Or just to GCC 14, hmm. Thoughts? * * * Another update - fixing an independent issue which makes sense to be part of this patch. Allocatable/pointer scalars are currently mapped as: #pragma omp target enter data map(to:*var.1_1 [len: 4]) map(alloc:var [pointer assign, bias: 0]) #pragma omp target exit data map(from:*var.2_2 [len: 4]) where 'GOMP_MAP_POINTER' is removed in gimplify.cc. In v3 (and v4) of this patch, this kind of handling moved from gimplify.cc to fortran/trans-openmp.cc; however, v3 has the same problem. For allocatable arrays, we have PSET + POINTER and the PSET part is changed/set to RELEASE/DELETE for 'exit data' But for scalars, the map was still left on the stack. Besides having a stale map, this could lead to fails when the stack was popped, especially when attempting to later map another stack variable with the same stack address, partially overlapping with the stale POINTER. Side remark: I found this for testcase that is part of an upcoming deep-mapping follow-up patch; that test failed with -O1 but worked with -O0/-Og due to changed stack usage. (Deep-mapping of allocatable components is on the OG12 branch; it is scheduled for mainline integration after stage1 opened.) The updated mainline patch is included; map-10.f90 is the new testcase. If anyone wants to see it separately, the patch has been committed to OG12 as https://gcc.gnu.org/g:8ea805840200f7dfd2c11b37abf5fbfe479c2fe2 Comments/thoughts/remarks to this patch? Tobias PS: For the rest of the patch, see a short description below - or with some longer remarks previous in this thread. On 27.02.23 13:15, Tobias Burnus wrote: And another re-diff for GCC 13/mainline, updating gcc/testsuite/ (The last change is related to the "[OG12,committed] Update dg-dump-scan for ..." discussion + OG12 https://gcc.gnu.org/g:e4de87a2309 / https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612871.html ) On 23.02.23 17:42, Tobias Burnus wrote: On 21.02.23 12:57, Tobias Burnus wrote: This patch moves some generic code for Fortran out of gimplify.cc to trans-openmp.cc and fixes several issues related to mapping. Tested with nvptx offloading. OK for mainline? Tobias Caveats: Besides the issues shown in the comment-out code, there remains also an issue with implicit mapping - at least for deferred-length strings, but I wouldn't be surprised if - at least depending on the used 'defaultmap' value (e.g. 'alloc') - there are also issues with array descriptors. Note: Regarding the declare target check for mapping: Without declare target, my assumption is that the hidden length variable will get implicitly mapped if needed. Independent of deferred-length or not, there is probably an issue with 'defaultmap(none)' and the hidden variable. - In any case, I prefer to defer all those issues to later (by having them captured in one/several PR). Tobias PS: This patch is a follow up to [Patch] Fortran/OpenMP: Fix DT struct-component with 'alloc' and array descr https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604887.html which fixed part of the problems. But as discussed on IRC, it did treat 'alloc' as special and missed some other map types. - In addition, this patch has a much extended test coverage and fixes some more issues found that way. - 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 Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings Previously, array descriptors might have been mapped as 'alloc' instead of 'to' for 'alloc', not updating the array bounds. The 'alloc' could also appear for 'data exit', failing with a libgomp assert. In some cases, either array descriptors or deferred-length string's length variable was not mapped. And, finally, some offset calculations with array-sections mappings went wrong. Additionally, the patch now unmaps for scalar allocatables/pointers the GOMP_MAP_POINTER, avoiding stale mappings. The testcases contain some comment-out tests which require follow-up work and for which PR exist. Those mostly relate to deferred-length strings which have several issues beyong OpenMP support. gcc/fortran/ChangeLog: * trans-decl.cc (gfc_get_symbol_decl): Add attributes such as 'declare target' also to hidden artificial variable for deferred-length character variables. * trans-openmp.cc (gfc_trans_omp_array_section, gfc_trans_omp_clauses, gfc_
[OG12][committed] Fortran: Add attr.class_ok check for generate_callback_wrapper
On OG12, the OpenMP deep-mapping support added a callback procedure to the
vtable.
That one did not handle error recovery well (ICE when a CLASS component as not
(class_)ok.
The attached patch has been committed as
https://gcc.gnu.org/g:9c18db65914a751e4a1d9330ccc1659fe5ef270d
and applies only to OG12 (= git branch devel/omp/gcc-12) as mainline does not
have this
code (yet).
* * *
The plan is to upstream the deep-mapping support, i.e. mapping of allocatable
components.
The current OG12 implementation handles both mapping the declared type and the
dynamic type,
the latter requires the wrapper, generated by generate_callback_wrapper.
I plan to upstream first the static part - and only then think about the
wrapper. I think
the wrapper could be useful for coarrays as well - namely, for the user-defined
reduction,
but I have not fully thought about it. It would break the ABI as the vtable
gets another
entry before the type-bound procedures, which is why I am a bit hesitant; it it
gets merged,
we it would be the opportunity to change some other things as well - like:
generating the
CLASS functions/vtable only when used. (→ weak symbols to permit it in multiple
translation
units; storing the fact that it has been generated in the module.)
But that's offtopic.
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
commit 9c18db65914a751e4a1d9330ccc1659fe5ef270d
Author: Tobias Burnus
Date: Thu Mar 23 14:29:00 2023 +0100
Fortran: Add attr.class_ok check for generate_callback_wrapper
Proper variables/components of type BT_CLASS have 'class_ok' set; check
for that to avoid an ICE on invalid code for gfortran.dg/pr108434.f90.
gcc/fortran/
* class.cc (generate_callback_wrapper): Add attr.class_ok check.
* resolve.cc (resolve_fl_derived): Likewise.
diff --git a/gcc/fortran/ChangeLog.omp b/gcc/fortran/ChangeLog.omp
index 663102d9329..f7d1f91f178 100644
--- a/gcc/fortran/ChangeLog.omp
+++ b/gcc/fortran/ChangeLog.omp
@@ -1,3 +1,8 @@
+2023-03-23 Tobias Burnus
+
+ * class.cc (generate_callback_wrapper): Add attr.class_ok check.
+ * resolve.cc (resolve_fl_derived): Likewise.
+
2023-03-23 Tobias Burnus
* trans-openmp.cc (gfc_trans_omp_clauses): Fix unmapping of
diff --git a/gcc/fortran/class.cc b/gcc/fortran/class.cc
index 35dc35d2ee6..7ab6923523f 100644
--- a/gcc/fortran/class.cc
+++ b/gcc/fortran/class.cc
@@ -2550,6 +2550,9 @@ generate_callback_wrapper (gfc_symbol *vtab, gfc_symbol *derived,
cb (token, comp->var(.data), size, 0, var's cb fn); */
for (gfc_component *comp = derived->components; comp; comp = comp->next)
{
+ if (__builtin_expect (comp->ts.type == BT_CLASS
+ && !comp->attr.class_ok, 0))
+ continue;
bool pointer = (comp->ts.type == BT_CLASS
? CLASS_DATA (comp)->attr.pointer : comp->attr.pointer);
bool proc_ptr = comp->attr.proc_pointer;
@@ -2590,7 +2593,7 @@ generate_callback_wrapper (gfc_symbol *vtab, gfc_symbol *derived,
size->where = gfc_current_locus;
}
- if (!proc_ptr && comp->ts.type == BT_CLASS)
+ if (!proc_ptr && comp->ts.type == BT_CLASS && comp->attr.class_ok)
{
gfc_add_data_component (expr);
if (comp->attr.dimension)
diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index aaeaf396b91..15db1252366 100644
--- a/gcc/fortran/resolve.cc
+++ b/gcc/fortran/resolve.cc
@@ -15173,7 +15173,8 @@ resolve_fl_derived (gfc_symbol *sym)
gfc_component *c = (sym->attr.is_class
? CLASS_DATA (sym->components) : sym->components);
for ( ; c; c = c->next)
-if ((c->ts.type == BT_DERIVED || c->ts.type == BT_CLASS)
+if ((c->ts.type == BT_DERIVED
+ || (c->ts.type == BT_CLASS && c->attr.class_ok))
&& !c->ts.u.derived->resolve_symbol_called)
{
if (c->ts.u.derived->components == NULL
[OG12][committed] Fortran/OpenMP: Fix 'alloc' and 'from' mapping for allocatable components
This is about OpenMP's "deep mapping" of allocatable components of derived
types.
The basic feature is on OG12 (and OG11) but yet in GCC mainline. The old
submissions are at
https://gcc.gnu.org/pipermail/gcc-patches/2022-April/593704.html
My plan is to get the whole feature into GCC 14 once trunk has opened (and
after some simpler pending patches have been merged). It requires some
re-diffing to be more digestible.
* * *
OG12: This patch as been committed to the devel/omp/gcc-12 branch as
https://gcc.gnu.org/g:a63735b8034db65a33c359633462accd9d71d3b5
* * *
This patch fixes an issue with 'map(alloc:' and 'map(from:' with
deep mapping of allocatable components - namely:
* For unmapping/coping to the host, the state of unallocated allocatables
needs to be preservered.
* For mapping to the device ('alloc' and 'from'), we still need to copy
data to the device to have the array bounds correctly set.
The data pointer (of allocated allocatables) is set as part of allocating
memory on the device ('attach'); thus, this part works.
As described in the patch (cf. comment above the checking function), we
could either copy only the descriptor data (and the NULL for pointers)
or we copy everything (shallowly) which includes this data. As there is
no means to do the former (without changing the refcount), we do the latter.
NOTE: The actual data to which the scalar/array allocatable points to is
not 'to' mapped but only 'alloc'. As that is supposed to be the large data,
copying everything should™ not cause a large performance penalty with real-world
code; it could be even faster than, let's say, copying 5 descriptors separately.
OpenMP spec side: It is not completely clear how the OpenMP spec expects
the copy out to work. Hence, I filed OpenMP Spec Issue #3545.
-
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
commit a63735b8034db65a33c359633462accd9d71d3b5
Author: Tobias Burnus
Date: Thu Mar 23 18:04:17 2023 +0100
Fortran/OpenMP: Fix 'alloc' and 'from' mapping for allocatable components
Even with 'alloc' and map-entering 'from' mapping, the following should hold.
For explicit mapping, that's already the case, this handles the automatical
deep mapping of allocatable components. Namely:
* On the device, the array bounds (of allocated allocatables) must match the
host, implying 'to' (or 'tofrom') mapping.
* On map exiting, the copying out shall not destroy the unallocated allocation
status (nor the pointer address of allocated allocatables).
The latter was not a problem for allocated allocatables as for those a pointer
was GOMP_MAP_ATTACHed; however, for unallocated allocatables, before it copied
back device-allocated memory which might not be nullified.
While 'alloc' was not deep-mapped at all, for map-entering 'from', the array
bounds were not set, making allocated derived-type components inaccessible on
the device (and wrong on the host on copy back).
The solution is, first, to deep-map 'alloc' as well and to copy to the device
even with 'alloc' and (map-entering) 'from'. This copying is only done if there
is a scalar (for the unallocated case) or array allocatable directly in the
derived type and then it is shallowly copied; the data pointed to is then again
only alloc'ed, unless it contains in turn allocatables.
gcc/fortran/
* trans-openmp.cc (gfc_has_alloc_comps): Add 'bool
shallow_alloc_only=false' arg.
(gfc_omp_replace_alloc_by_to_mapping): New, call it.
(gfc_omp_deep_map_kind_p): Return 'true' also for '(present,)alloc'.
(gfc_omp_deep_mapping_item, gfc_omp_deep_mapping_do): On map entering,
replace shallowly 'alloc'/'from' by '(from)to' mapping if there are
allocatable components.
libgomp/
* testsuite/libgomp.fortran/map-alloc-comp-8.f90: New test.
---
gcc/fortran/ChangeLog.omp | 10 +
gcc/fortran/trans-openmp.cc| 96 +++-
libgomp/ChangeLog.omp | 4 +
.../testsuite/libgomp.fortran/map-alloc-comp-8.f90 | 268 +
4 files changed, 371 insertions(+), 7 deletions(-)
diff --git a/gcc/fortran/ChangeLog.omp b/gcc/fortran/ChangeLog.omp
index f7d1f91f178..e3ab2254215 100644
--- a/gcc/fortran/ChangeLog.omp
+++ b/gcc/fortran/ChangeLog.omp
@@ -1,3 +1,13 @@
+2023-03-23 Tobias Burnus
+
+ * trans-openmp.cc (gfc_has_alloc_comps): Add 'bool
+ shallow_alloc_only=false' arg.
+ (gfc_omp_replace_alloc_by_to_mapping): New, call it.
+ (gfc_omp_deep_map_kind_p): Return 'true' also for '(present,)alloc'.
+ (gfc_omp_deep_mapping_item, gfc_omp_deep_mapping_do): On map entering,
+ repla
Re: [PATCH][stage1] Remove conditionals around free()
On Fri, Mar 3, 2023 at 10:14 PM Jerry D via Fortran wrote: > I am certainly not a C++ expert but it seems to me this all begs for > automatic finalization where one would not have to invoke free at all. > I suspect the gfortran frontend is not designed for such things. +1 for RAII
