[Mlir-commits] [flang] [mlir] [mlir][OpenMP][flang] make private variable allocation implicit in omp.private (PR #124019)

[Sergio Afonso]

================
@@ -184,7 +218,8 @@ bool DataSharingProcessor::needBarrier() {
   // Emit implicit barrier for linear clause. Maybe on somewhere else.
   for (const semantics::Symbol *sym : allPrivatizedSymbols) {
     if (sym->test(semantics::Symbol::Flag::OmpLastPrivate) &&
-        (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) || callsInitClone))
+        (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) ||
+         mightHaveReadHostSym))
----------------
skatrak wrote:

Sorry @tblah for the late comment to this PR. I am looking into an issue that came up recently, and I believe it's related to this check here and your extensions to initialize that flag also during delayed privatization. So, I wanted to ask about it to try to understand it better, and see what the best solution is.

As far as I can tell, the `mightHaveReadHostSym` flag is set globally. That is, whenever a symbol is known to either initialize a clone or to have a privatizer with an init mold argument that is used, this will be true. Then, a barrier will be inserted e.g. if it is true and there are any `lastprivate` symbols. Is that the intended behavior?

My concern is that it doesn't look at whether the `lastprivate` symbol is the same one that caused the flag to be set. This is something that is triggered in the following example:

```f90
subroutine simd()
  integer :: i, j
  integer, dimension ( 3 ) :: x

  !$omp simd collapse(2) private(x)
  do i = 1, 10
    do j = 1, 10
      call foo(x)
    end do
  end do
  !$omp end simd
end subroutine simd
```

The `i` variable is implicitly marked as `lastprivate` (not sure where that's done, but I'm assuming it's `simd`-specific behavior) and there is a `private` clause for a non-primitive type, so this results in the insertion of a barrier. However, replacing `simd` for `do` no longer causes this issue because `i` is no longer `lastprivate` then. I was just wondering if it's on purpose that we're adding a barrier due to different symbols meeting each side of the condition.

I guess that would have been triggered previously as well, it's just that this change made it also happen for delayed privatization, which then impacts composite construct lowering and ends up triggering an MLIR verifier error for target SPMD due to the added barrier.

Maybe @ergawy or @luporl can also chime in if you know whether this is the expected behavior. I have a small patch updating `mightHaveReadHostSym` so that a barrier would not be inserted in this case, which I can share if this isn't expected behavior.

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