[flang-commits] [flang] [flang][OpenMP] Fix privatization of threadprivate common block (PR #77821)

[Leandro Lupori via flang-commits]

https://github.com/luporl updated https://github.com/llvm/llvm-project/pull/77821

>From 1e1b515a7fbcbb717550a36eb2603ee72ce484b3 Mon Sep 17 00:00:00 2001
From: Leandro Lupori <leandro.lupori at linaro.org>
Date: Thu, 11 Jan 2024 16:17:19 -0300
Subject: [PATCH 1/2] [flang][OpenMP] Fix privatization of threadprivate common
 block

In some cases, when privatizing a threadprivate common block, the
original symbol will correspond to the common block, instead of
its threadprivate version. This can happen, for instance, with a
common block, declared in a separate module, used by a parent
procedure and privatized in its child procedure. In this case,
symbol lookup won't find a symbol in the parent procedure, but
only in the module where the common block was defined.

Fixes https://github.com/llvm/llvm-project/issues/65028
---
 flang/lib/Lower/OpenMP.cpp                    | 20 +++++++------
 .../OpenMP/threadprivate-commonblock-use.f90  | 29 +++++++++++++++++++
 2 files changed, 40 insertions(+), 9 deletions(-)
 create mode 100644 flang/test/Lower/OpenMP/threadprivate-commonblock-use.f90

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index c3a570bf15ea0..ce2ee4befddcf 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -2029,18 +2029,20 @@ static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
   mlir::OpBuilder::InsertPoint insPt = firOpBuilder.saveInsertionPoint();
   firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
 
-  // Get the original ThreadprivateOp corresponding to the symbol and use the
-  // symbol value from that operation to create one ThreadprivateOp copy
-  // operation inside the parallel region.
+  // If the symbol corresponds to the original ThreadprivateOp, use the symbol
+  // value from that operation to create one ThreadprivateOp copy operation
+  // inside the parallel region.
+  // In some cases, however, the symbol will correspond to the original,
+  // non-threadprivate variable. This can happen, for instance, with a common
+  // block, declared in a separate module, used by a parent procedure and
+  // privatized in its child procedure.
   auto genThreadprivateOp = [&](Fortran::lower::SymbolRef sym) -> mlir::Value {
-    mlir::Value symOriThreadprivateValue = converter.getSymbolAddress(sym);
-    mlir::Operation *op = symOriThreadprivateValue.getDefiningOp();
+    mlir::Value symValue = converter.getSymbolAddress(sym);
+    mlir::Operation *op = symValue.getDefiningOp();
     if (auto declOp = mlir::dyn_cast<hlfir::DeclareOp>(op))
       op = declOp.getMemref().getDefiningOp();
-    assert(mlir::isa<mlir::omp::ThreadprivateOp>(op) &&
-           "Threadprivate operation not created");
-    mlir::Value symValue =
-        mlir::dyn_cast<mlir::omp::ThreadprivateOp>(op).getSymAddr();
+    if (mlir::isa<mlir::omp::ThreadprivateOp>(op))
+      symValue = mlir::dyn_cast<mlir::omp::ThreadprivateOp>(op).getSymAddr();
     return firOpBuilder.create<mlir::omp::ThreadprivateOp>(
         currentLocation, symValue.getType(), symValue);
   };
diff --git a/flang/test/Lower/OpenMP/threadprivate-commonblock-use.f90 b/flang/test/Lower/OpenMP/threadprivate-commonblock-use.f90
new file mode 100644
index 0000000000000..28616f7595a0d
--- /dev/null
+++ b/flang/test/Lower/OpenMP/threadprivate-commonblock-use.f90
@@ -0,0 +1,29 @@
+! This test checks lowering of OpenMP Threadprivate Directive.
+! Test for common block, defined in one module, used in a subroutine of
+! another module and privatized in a nested subroutine.
+
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK: fir.global common @cmn_(dense<0> : vector<4xi8>) : !fir.array<4xi8>
+module m0
+  common /cmn/ k1
+  !$omp threadprivate(/cmn/)
+end
+
+module  m1
+contains
+  subroutine ss1
+    use m0
+  contains
+!CHECK-LABEL: func @_QMm1Fss1Pss2
+!CHECK: %[[CMN:.*]] = fir.address_of(@cmn_) : !fir.ref<!fir.array<4xi8>>
+!CHECK: omp.parallel
+!CHECK: %{{.*}} = omp.threadprivate %[[CMN]] : !fir.ref<!fir.array<4xi8>> -> !fir.ref<!fir.array<4xi8>>
+    subroutine ss2
+      !$omp parallel copyin (k1)
+      !$omp end parallel
+    end subroutine ss2
+  end subroutine ss1
+end
+
+end

>From 79e367391baed7dc398ae4b74d8e0077f39faea5 Mon Sep 17 00:00:00 2001
From: Leandro Lupori <leandro.lupori at linaro.org>
Date: Fri, 2 Feb 2024 15:03:02 +0000
Subject: [PATCH 2/2] Ensure genThreadprivateOp's symbols are threadprivate

---
 flang/lib/Lower/OpenMP.cpp | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index ce2ee4befddcf..2db1a776b9984 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -2022,6 +2022,19 @@ static fir::ExtendedValue getExtendedValue(fir::ExtendedValue base,
       });
 }
 
+#ifndef NDEBUG
+static bool isThreadPrivate(Fortran::lower::SymbolRef sym) {
+  if (const auto *details =
+          sym->detailsIf<Fortran::semantics::CommonBlockDetails>()) {
+    for (const auto &obj : details->objects())
+      if (!obj->test(Fortran::semantics::Symbol::Flag::OmpThreadprivate))
+        return false;
+    return true;
+  }
+  return sym->test(Fortran::semantics::Symbol::Flag::OmpThreadprivate);
+}
+#endif
+
 static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
                                 Fortran::lower::pft::Evaluation &eval) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
@@ -2037,6 +2050,7 @@ static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
   // block, declared in a separate module, used by a parent procedure and
   // privatized in its child procedure.
   auto genThreadprivateOp = [&](Fortran::lower::SymbolRef sym) -> mlir::Value {
+    assert(isThreadPrivate(sym));
     mlir::Value symValue = converter.getSymbolAddress(sym);
     mlir::Operation *op = symValue.getDefiningOp();
     if (auto declOp = mlir::dyn_cast<hlfir::DeclareOp>(op))