[committed] Re: [Patch,v4] Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings
The patch has now been committed as r14-931-g80bb0b8a81fdc5 The only change is that I added the &_P in 'if (sym->ts.deferred && VAR_P (length))' in trans-decl.cc just to avoid potential issues in case length is not a var decl (but e.g. a '0' tree node, cf. code). Tobias On 23.03.23 10:28, Tobias Burnus wrote: [...] 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 commit 80bb0b8a81fdc5d0a1c88ae3febd593868daa752 Author: Tobias Burnus Date: Wed May 17 12:28:14 2023 +0200 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
[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,
