[flang-commits] [flang] [OpenMP][Flang] Add "IsolatedFromAbove" trait to omp.target (PR #67164)

[via flang-commits]

agozillon wrote:

> > > Perhaps I am misunderstanding what you mean (so my apologies if I am), but I believe this is already the case, map_info holds or is supposed to hold bounds information: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L1149.
> > > It is a little weird that we'd now end up creating a map_info for each bound which then has an empty bound though. I agree that it would be nice to not have to do that as it feels a bit like it defeats the purpose of a map_info owning a bound, but perhaps it's unavoidable.
> > 
> > 
> > What i am saying is, for the Fortran+OpenMP example below, the `map_info` generated should be the same as if the target directive was `!$omp target map(a(11,22))` or `!$omp target map(a)`. If we do it that way then all the bounds will be part of the `map_info` of `a(11,22)` or `a`. At the moment, I don't see bounds generated. I believe the relevant entry is the following and it does not have an `omp.bounds` for the `omp.map_info`.
> > ```
> >    !CHECK: %[[VAL_6:.*]] = omp.map_info var_ptr(%[[VAL_5]]#1 : !fir.ref<!fir.array<?x1024xi32>>)   map_clauses(literal, implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<!fir.array<?x1024xi32>> {name = "a"}
> > ```
> > 
> > 
> > ```
> >    integer :: n
> >    integer :: a(n, 1024)
> >    !$omp target 
> >    a(11, 22) = 33
> >    !$omp end target
> > ```
> 
> This map operation is indeed missing bounds and this is not correct/expected.
> 
> This example also shows that it is concerning making all mapping decisions so early in order to satisfy the IsolatedFromAbove property. Namely, this case would ideally be map(tofrom:a(11,22)) not a map(tofrom:a). Maybe it is indeed possible for this case - but consider a simple loop modifying `a(11,22+ii)`. Wouldn't it be great to just map what is needed? Hard to get this info at lowering. https://www.openmp.org/spec-html/5.2/openmpsu60.html
> 
> I wonder if it should be considered to add the trait IsolatedFromAbove sometime after lowering and after running the passes that will decide values passed by reference/value and those that decide the mapping actions for implicit data mapping attributes.

I think this begins to lean towards approach 3) that @jeanPerier mentioned initially as a possible approach (maybe I'm wrong though) where we'd effectively end up with something similar to the EarlyOutliningPass we have at the moment, but slightly different, as it'd have to enforce that the target region is IsolatedFromAbove before applying it rather than cloning to a different function. Not against it just an observation in this case. That is a very good example thank you! 

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