[Mlir-commits] [flang] [openmp] [mlir] [Flang][OpenMP] Initial mapping of Fortran pointers and allocatables for target devices (PR #71766)

[llvmlistbot at llvm.org]

agozillon wrote:

> One more thing to note before I get back to this review later this week and actually comment on the mapping approach itself after reading your explanations. I want to point the following since your explanations and even your new field seem to be focused around pointers and allocatables.
> 
> There are other cases where descriptors are created outside of pointer and allocatable. For example, Fortran assumed-shape arrays [1]. Since the access to the data is also lowered as an access through descriptor in FIR, such a descriptor also needs mapped to device. I will use OpenACC since I would have to read through OpenMP spec more deeply to understand if behavior should match. [1] https://fortran-lang.org/en/learn/best_practices/arrays/
> 
> Consider the following OpenACC code:
> 
> ```
> subroutine mapper(array)
>   integer :: array(:)
>   !$acc serial copy(array)
>   array(1) = 1
>   !$acc end serial
> end subroutine
> ```

Yes, I ran into this case recently and I'm looking into it just now, so there's a good chance this PR will change a bit soon. So no extreme rush to review the PR in detail if you're too busy! but I am always happy to take more input that nudges the PR in the right direction.

> The acc dialect represents this like so (I am only pasting relevant parts) :
> 
> ```
> func.func @_QPmapper(%arg0: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "array"}) {
>   ...
>   %0 = fir.declare %arg0 {uniq_name = "_QFmapperEarray"} : (!fir.box<!fir.array<?xi32>>) -> !fir.box<!fir.array<?xi32>>
>   %1 = fir.rebox %0 : (!fir.box<!fir.array<?xi32>>) -> !fir.box<!fir.array<?xi32>>
>   ...
>   %5 = fir.box_addr %1 : (!fir.box<!fir.array<?xi32>>) -> !fir.ref<!fir.array<?xi32>>
>   %6 = acc.copyin varPtr(%5 : !fir.ref<!fir.array<?xi32>>) bounds(%4) -> !fir.ref<!fir.array<?xi32>> {dataClause = #acc<data_clause acc_copy>, name = "array"}
>   acc.serial dataOperands(%6 : !fir.ref<!fir.array<?xi32>>) {
>     %7 = fir.array_coor %1 %c1 : (!fir.box<!fir.array<?xi32>>, index) -> !fir.ref<i32>
>     ...
> ```
> 
> The relevant part is that the descriptor.base_addr is what is in the operation that represents data clause. But in the region itself, the descriptor is accessed to get to the data. So the descriptor needs mapped implicitly. What I wanted to note here is that the acc dialect data operations (and as a result omp.map_info) were designed to allow decomposed mapping operations to allow mapping of anything needed in compute region.
> 
> So it is possible to represent mapping of descriptor without adding a new field. Like so:
> 
> ```
>   %0 = fir.alloca !fir.box<!fir.array<?xi32>>
>   %1 = fir.declare %arg0 {uniq_name = "_QFmapperEarray"} : (!fir.box<!fir.array<?xi32>>) -> !fir.box<!fir.array<?xi32>>
>   %2 = fir.rebox %1 : (!fir.box<!fir.array<?xi32>>) -> !fir.box<!fir.array<?xi32>>
>   fir.store %2 to %0 : !fir.ref<!fir.box<!fir.array<?xi32>>>
>   ...
>   %6 = fir.load %0 : !fir.ref<!fir.box<!fir.array<?xi32>>>
>   %7 = fir.box_addr %6 : (!fir.box<!fir.array<?xi32>>) -> !fir.ref<!fir.array<?xi32>>
>   %8 = acc.copyin varPtr(%7 : !fir.ref<!fir.array<?xi32>>) bounds(%5) -> !fir.ref<!fir.array<?xi32>> {dataClause = #acc<data_clause acc_copy>, name = "array"}
>   %9 = acc.copyin varPtr(%0 : !fir.ref<!fir.box<!fir.array<?xi32>>>) -> !fir.ref<!fir.box<!fir.array<?xi32>>> {implicit = true, name = "array descriptor"}
>   acc.serial dataOperands(%9, %8 : !fir.ref<!fir.box<!fir.array<?xi32>>>, !fir.ref<!fir.array<?xi32>>) {
> ```
> 
> Two things to note - we have to create storage since the acc dialect requires ptr type (and so does omp dialect). And the other is that the copy of descriptor would just be another acc.copyin operation without the need to add any new flags.

Yes, I tried this originally, although I was utilising the varPtrPtr field inappropriately to create some concept of a parent -> member mapping, to help create the offloading information required to bind the data and descriptor together, so that it mimics a mapping similar to: 

map(to: descriptor, tofrom: descriptor->data)

Where, descriptor is the structure, and the held allocated data is a pointer member of the structure that will be attached (still horrible with OpenMP/OpenACC wording, my apologies if it's incorrect). Perhaps an alternative mapping is possible though or perhaps it's possible to correlate the two map_info as being related without the extra field being added to map_info (although, probably a parent field, would be less contentious than the is_fortran_allocatable field (probably should be named has_fortran_descriptor in hindsight)). 

https://github.com/llvm/llvm-project/pull/71766