[flang-commits] [flang] [flang][MLIR][OpenMP] Extend delayed privatization for arrays (PR #85023)

[Kareem Ergawy via flang-commits]

ergawy wrote:

> Thank you for the patch. I was trying to understand the changes with simpler examples of the form:
> 
> ```
> program main
>     implicit none
>     integer :: i
>     real, dimension(5) :: array
> 
>     !$omp parallel private(array)
>        array(2) = array(2) + 10
>     !$omp end parallel
> end program
> ```
> 
> It turns out that with `-mmlir --openmp-enable-delayed-privatization` flag, we get the following verification failure:
> 
> ```
> error: loc("/home/user1/.llvm/test/par.f90":4:27): 'hlfir.declare' op of array entity with a raw address base must have a shape operand that is a shape or shapeshift
> error: loc("/home/user1/.llvm/test/par.f90":4:27): 'fir.shape' op using value defined outside the region
> error: loc("/home/user1/.llvm/test/par.f90":4:27): 'hlfir.declare' op of array entity with a raw address base must have a shape operand that is a shape or shapeshift
> ```
> 
> I am not completely sure what went wrong here.

After debugging this for a bit, it seems the issue only happens if we use the default lower bound (i.e. if we do not specify an explicit non-default lower bound for the array). For example, the issue does not happen if you change the example you provided to the following:
```
 program main
     implicit none
     integer :: i
     real, dimension(2:6) :: array
 
     !$omp parallel private(array)
        array(2) = array(2) + 10
     !$omp end parallel
 end program
```

Looking at the IR, it seems flang generates the `hlfir.declare` op differently for the `dimension(5) :: array` case vs the `dimension(2:6) :: array` case.

For `dimension(5) :: array`, the following `hlfir.declare` op is generated:
```mlir
%3:2 = "hlfir.declare"(%1, %2) <{operandSegmentSizes = array<i32: 1, 1, 0>, uniq_name = "_QFEarray"}>
  : (!fir.ref<!fir.array<5xf32>>, !fir.shape<1>)
    -> (!fir.ref<!fir.array<5xf32>>, !fir.ref<!fir.array<5xf32>>)
```

For `dimension(2:6) :: array`, the following `hlfir.declare` op is generated:
```mlir
%4:2 = "hlfir.declare"(%2, %3) <{operandSegmentSizes = array<i32: 1, 1, 0>, uniq_name = "_QFEarray"}>
  : (!fir.ref<!fir.array<5xf32>>, !fir.shapeshift<1>)
    -> (!fir.box<!fir.array<5xf32>>, !fir.ref<!fir.array<4xf32>>)
```

In the first case (i.e. `dimension(5) :: array`), note that `hlfir.declare` returns an **unboxed** value: it returns a direct reference to the array. In the second case, `hlfir.declare` returns a **boxed** value which is what we need here.

For me (and take this with a grain of salt since I didn't work on this part of the code base before) this seems like a bug in how `hlfir.declare` is generated: I think we should box the value regardless of whether it has the default or a non-default lower bound specified for the array.

@jeanPerier I think you will have a more informed input here. Can you please take a look and let me know what you think?

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