[flang-commits] [flang] [Flang][MLIR] - Access the LEN for a `fir.boxchar<k>` and use it to set the bounds `omp.map.info` ops. (PR #134967)

[Slava Zakharin via flang-commits]

================
@@ -268,8 +274,54 @@ static void bindEntryBlockArgs(lower::AbstractConverter &converter,
                                             cloneBounds(v.getLBounds())));
           },
           [&](const fir::CharBoxValue &v) {
-            converter.bindSymbol(
-                sym, fir::CharBoxValue(arg, cloneBound(v.getLen())));
+            // In some cases, v.len could reference the input to the
+            // hlfir.declare which is the corresponding v.addr. While this isn't
+            // a big problem by itself, it is desirable to extract this out of
+            // v.addr itself since it's first result will be of type
+            // fir.boxchar<>. For example, consider the following
+            //
+            // func.func private @_QFPrealtest(%arg0: !fir.boxchar<1>)
+            //  %2 = fir.dummy_scope : !fir.dscope
+            //  %3:2 = fir.unboxchar %arg0 : (!fir.boxchar<1>) ->
+            //         (!fir.ref<!fir.char<1,?>>, index)
+            //  %4:2 = hlfir.declare (%3#0, %3#1, %2):(!fir.ref<!fir.char<1,?>>,
+            //                        index,!fir.dscope) ->
+            //                       (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
+
+            // In the case above,
+            // v.addr is
+            //  %4:2 = hlfir.declare (%3#0, %3#1, %2):(!fir.ref<!fir.char<1,?>>,
+            //                        index,!fir.dscope) ->
+            //                       (!fir.boxchar<1>, !fir.ref<!fir.char<1,?>>)
+            // v.len is
+            //  %3:2 = fir.unboxchar %arg0 : (!fir.boxchar<1>) ->
+            //         (!fir.ref<!fir.char<1,?>>, index)
+
+            // Mapping this to the target will create a use of %arg0 on the
+            // target. Since omp.target is IsolatedFromAbove, %arg0 will have to
----------------
vzakhari wrote:

I think I see it now that `cloneBound` clones the defining operation given one of its results.  It was also assuming that the cloned result is `#0` of the operation, which is wrong, and you fixed it for `UnboxCharOp`.

This looks broken to me.  I think what we really want to pass to the OpenMP region is the scalar bound value, and this can be done without cloning the whole defining operation.

The bounds should be scalars, as I understand, so it should be unnecessary to clone the char box to get the bound value.

Consider how this case works:
```
subroutine TestOfCharacter(a0, a1, l)
  integer, intent(in) :: l
  real, intent(in) :: a0(l:)
  real, intent(inout):: a1(l:)

  !$omp target map(to:a0) map(from: a1)
  a1 = a0
  !$omp end target
end subroutine TestOfCharacter
```

The lower bounds are stored into temporaries for passing them via implicit maps to the target region:
```
    %0 = fir.alloca i32
    %1 = fir.alloca i32
    %2 = fir.dummy_scope : !fir.dscope
    %3:2 = hlfir.declare %arg2 dummy_scope %2 {fortran_attrs = #fir.var_attrs<intent_in>, uniq_name = "_QFtestofcharacterEl"} : (!fir.ref<i32>, !fir.dscope) -> (!fir.ref<i32>, !fir.ref<i32>)
    %4 = fir.load %3#0 : !fir.ref<i32>
    fir.store %4 to %0 : !fir.ref<i32>
...
    %9 = fir.load %3#0 : !fir.ref<i32>
    fir.store %9 to %1 : !fir.ref<i32>
...
    %24 = omp.map.info var_ptr(%1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
    %25 = omp.map.info var_ptr(%0 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = ""}
    omp.target map_entries(%18 -> %arg3, %23 -> %arg4, %24 -> %arg5, %25 -> %arg6 : !fir.ref<!fir.array<?xf32>>, !fir.ref<!fir.array<?xf32>>, !fir.ref<i32>, !fir.ref<i32>) {
      %26 = fir.load %arg6 : !fir.ref<i32>
      %27 = fir.load %arg5 : !fir.ref<i32>
```

I do not know exactly where this happens, but I think the character length should be handled the same way.

Do you know where this array lower bounds handling happens?

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