Hi Julian,
I think this patch is OK; however, at least for gimplify.cc Jakub needs to have
a second look.
As remarked for the 2/4 patch, I believe mapping 'map(tofrom: var%f(2:3))'
should work
without explicitly mapping 'map(tofrom: var%f)'
(→ [TR11 157:21-26] (approx. [5.2 154:22-27], [5.1 352:17-22], [5.0 320:22-27]).
→ https://gcc.gnu.org/pipermail/gcc-patches/2022-December/608100.html (+
previously in the thread).
Testing the patch, that seems to work fine (i.e. contrary to C/C++, cf. 2/4),
which matches the dump and, if I understood correctly, also your (Julian's)
expectation.
Thus, no need to modify the code part.
Regarding the testcases:
* I would prefer if you don't modify the existing
libgomp.fortran/struct-elem-map-1.f90 testcase;
However, you could add your version as another variant ('subroutine nine()',
'four_var()' or
what's the next free name, possibly with a comment telling that it is
'four()' but with an
added explicit basepointer mapping.).
* As the new version should map *less*, I wonder whether some
-fdump-tree-{original,gimple,omplower}
scan-dump-tree checks would be useful besides testing whether it works at run
time.
(Your decision regarding which tree, which testcases and whether at all.)
* Likewise, maybe a 'target enter/exit data' check? However, you might very
well run into my
'omp target data exit' issue, cf.
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/604887.html
(needs to be revised based on Jakub's comments; I think those were on IRC
only – the problem is that
not only 'alloc' is affected but also 'from' etc.)
On 18.10.22 12:39, Julian Brown wrote:
Implementing the "omp declare mapper" functionality, I noticed some
cases where handling of derived type members that are pointers doesn't
seem to be quite right. At present, a type such as this:
...
map(to: tvar%arrptr) map(tofrom: tvar%arrptr(3:8))
and then instead we should follow (OpenMP 5.2, 5.8.3 "map Clause"):
...
2) map(tofrom: tvar%arrptr(3:8) -->
GOMP_MAP_TOFROM *tvar%arrptr%data(3) (size 8-3+1, etc.)
GOMP_MAP_TO_PSET tvar%arrptr
GOMP_MAP_ATTACH_DETACH tvar%arrptr%data (bias 3, etc.)
...
Additionally, the next patch in the series adds a runtime diagnostic
for the (illegal) case where 'i' and 'j' are different.
2022-10-18 Julian Brown <jul...@codesourcery.com>
gcc/fortran/
* dependency.cc (gfc_omp_expr_prefix_same): New function.
* dependency.h (gfc_omp_expr_prefix_same): Add prototype.
* gfortran.h (gfc_omp_namelist): Add "duplicate_of" field to "u2"
union.
* trans-openmp.cc (dependency.h): Include.
(gfc_trans_omp_array_section): Use GOMP_MAP_TO_PSET unconditionally for
mapping array descriptors.
(gfc_symbol_rooted_namelist): New function.
(gfc_trans_omp_clauses): Check subcomponent and subarray/element
accesses elsewhere in the clause list for pointers to derived types or
array descriptors, and adjust or drop mapping nodes appropriately.
gcc/
* gimplify.cc (omp_tsort_mapping_groups): Process nodes that have
OMP_CLAUSE_MAP_RUNTIME_IMPLICIT_P set after those that don't.
(omp_accumulate_sibling_list): Adjust GOMP_MAP_TO_PSET handling.
Remove GOMP_MAP_ALWAYS_POINTER handling.
libgomp/
* testsuite/libgomp.fortran/map-subarray.f90: New test.
* testsuite/libgomp.fortran/map-subarray-2.f90: New test.
* testsuite/libgomp.fortran/map-subarray-3.f90: New test.
* testsuite/libgomp.fortran/map-subarray-4.f90: New test.
* testsuite/libgomp.fortran/map-subarray-6.f90: New test.
* testsuite/libgomp.fortran/map-subarray-7.f90: New test.
* testsuite/libgomp.fortran/map-subcomponents.f90: New test.
* testsuite/libgomp.fortran/struct-elem-map-1.f90: Adjust for
descriptor-mapping changes. Remove XFAIL.
...
--- a/libgomp/testsuite/libgomp.fortran/struct-elem-map-1.f90
+++ b/libgomp/testsuite/libgomp.fortran/struct-elem-map-1.f90
@@ -229,7 +229,8 @@ contains
! !$omp target map(tofrom: var%d(4:7), var%f(2:3), var%str2(2:3)) &
! !$omp& map(tofrom: var%str4(2:2), var%uni2(2:3), var%uni4(2:2))
- !$omp target map(tofrom: var%d(4:7), var%f(2:3), var%str2(2:3),
var%uni2(2:3))
+ !$omp target map(to: var%f) map(tofrom: var%d(4:7), var%f(2:3), &
+ !$omp& var%str2(2:3), var%uni2(2:3))
This adds 'to: var%f' (to the existing 'var%f(2:3)') – where 'f' is a
POINTER. As discussed at the top, I prefer to leave it as is – and
possibly just add another test-function, replicating this function and
only there adding the basepointer as additional list item.
- !$omp target map(tofrom: var%f(2:3))
+ !$omp target map(to: var%f) map(tofrom: var%f(2:3))
likewise.
- !$omp target map(tofrom: var%d(5), var%f(3), var%str2(3), var%uni2(3))
+ !$omp target map(to: var%f) map(tofrom: var%d(5), var%f(3), &
+ !$omp& var%str2(3), var%uni2(3))
likewise.
- !$omp target map(tofrom: var%f(2:3))
+ !$omp target map(to: var%f) map(tofrom: var%f(2:3))
likewise.
Thanks,
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
