[flang][OpenMP] Don't privatize pre-determined symbols declare by nested BLOCKs

[ergawy]

Fixes a bug when a block variable is marked as pre-determined private. In such case, we can simply ignore privatizing that symbol within the context of the currrent OpenMP construct since the "private" allocation for the symbol will be emitted within the nested block anyway.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Kareem Ergawy (ergawy)

Changes

Fixes a bug when a block variable is marked as pre-determined private. In such case, we can simply ignore privatizing that symbol within the context of the currrent OpenMP construct since the "private" allocation for the symbol will be emitted within the nested block anyway.

---

Full diff: https://github.com/llvm/llvm-project/pull/152652.diff

3 Files Affected:

• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+10-2)
• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+12-4)
• (added) flang/test/Lower/OpenMP/block_predetermined_privatization.f90 (+32)

diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 67a9a4675f115..2ad2159bf806e 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -553,8 +553,16 @@ void DataSharingProcessor::collectSymbols(
       return sym->test(semantics::Symbol::Flag::OmpImplicit);
     }
 
-    if (collectPreDetermined)
-      return sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+    if (collectPreDetermined) {
+      // Collect pre-determined symbols only if they are defined by the current
+      // OpenMP evaluation. If `sym` is not defined by the current OpenMP
+      // evaluation then it is defined by a block nested within the OpenMP
+      // construct. This, in turn, means that the private allocation for the
+      // symbol will be emitted as part of the nested block and there is no need
+      // to prvatize it within the OpenMP construct.
+      return visitor.isSymbolDefineBy(sym, eval) &&
+             sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+    }
 
     return !sym->test(semantics::Symbol::Flag::OmpImplicit) &&
            !sym->test(semantics::Symbol::Flag::OmpPreDetermined);
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 96e7fa6d5d93c..9444b698244a0 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -58,13 +58,21 @@ class DataSharingProcessor {
     }
 
     void Post(const parser::Name &name) {
-      auto *current = !constructs.empty() ? constructs.back() : nullptr;
+      const void *current = !constructs.empty() ? constructs.back() : nullptr;
       symDefMap.try_emplace(name.symbol, current);
     }
 
-    llvm::SmallVector<const parser::OpenMPConstruct *> constructs;
-    llvm::DenseMap<semantics::Symbol *, const parser::OpenMPConstruct *>
-        symDefMap;
+    bool Pre(const parser::DeclarationConstruct &decl) {
+      constructs.push_back(&decl);
+      return true;
+    }
+
+    void Post(const parser::DeclarationConstruct &decl) {
+      constructs.pop_back();
+    }
+
+    llvm::SmallVector<const void *> constructs;
+    llvm::DenseMap<semantics::Symbol *, const void *> symDefMap;
 
     /// Given a \p symbol and an \p eval, returns true if eval is the OMP
     /// construct that defines symbol.
diff --git a/flang/test/Lower/OpenMP/block_predetermined_privatization.f90 b/flang/test/Lower/OpenMP/block_predetermined_privatization.f90
new file mode 100644
index 0000000000000..12346c14ef5d9
--- /dev/null
+++ b/flang/test/Lower/OpenMP/block_predetermined_privatization.f90
@@ -0,0 +1,32 @@
+! Fixes a bug when a block variable is marked as pre-determined private. In such
+! case, we can simply ignore privatizing that symbol within the context of the
+! currrent OpenMP construct since the "private" allocation for the symbol will
+! be emitted within the nested block anyway.
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+subroutine block_predetermined_privatization
+  implicit none
+  integer :: i
+
+  !$omp parallel
+  do i=1,10
+    block
+      integer :: j
+      do j=1,10
+      end do
+    end block
+  end do
+  !$omp end parallel
+end subroutine
+
+! CHECK-LABEL: func.func @_QPblock_predetermined_privatization() {
+! CHECK:         %[[I_DECL:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "{{.*}}Ei"}
+! CHECK:         omp.parallel private(@{{.*}}Ei_private_i32 %[[I_DECL]]#0 -> %{{.*}} : !fir.ref<i32>) {
+! CHECK:           fir.do_loop {{.*}} {
+! Verify that `j` is allocated whithin the same scope of its block (i.e. inside
+! the `parallel` loop).
+! CHECK:             fir.alloca i32 {bindc_name = "j", {{.*}}}
+! CHECK:           }
+! CHECK:         }
+! CHECK:       }

[mjklemm]

LGTM

[skatrak]

Thank you Kareem, LGTM as well.

[skatrak]

Would you be able to use an std::variant<const parser::OpenMPConstruct *, const parser::DeclarationConstruct *> instead? Mainly because I think having opaque pointers is not great for maintainability.

[luporl]

BTW, can't these be made private too?

[ergawy]

Done (variant and private).

[skatrak]

Suggested change

	// to prvatize it within the OpenMP construct.
	// to privatize it within the OpenMP construct.

[ergawy]

Done.

[luporl]

LGTM, but please consider extending the fix for implicit symbols.

[luporl]

If it doesn't break things, consider making this change for implicit symbols too, as they seem to have the same issue. Reproducer:

subroutine sub
  implicit none
  integer :: i

  !$omp task
  do i=1,10
    block
      integer :: j
      j = 0
    end block
  end do
  !$omp end task
end subroutine

[ergawy]

It breaks 2 tests:

  Flang :: Lower/OpenMP/implicit-dsa.f90
  Flang :: Lower/OpenMP/target.f90

So I will do that in a follow up PR.

[ergawy]

Done in #152973.

[luporl]

BTW, can't these be made private too?