Re: [PATCH v5 3/4] OpenMP: Pointers and member mappings

[Tobias Burnus <tobias@codesourcery.com>]

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  <julian@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