[flang][OpenMP] Generalize isOpenMPPrivatizingConstruct

[kparzysz]

Instead of treating all block and all loop constructs as privatizing, actually check if the construct allows a privatizing clause.

[kparzysz]

I have verified that 1052_0201 and 1052_0205 both pass with this change.

[llvmbot]

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

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

Instead of treating all block and all loop constructs as privatizing, actually check if the construct allows a privatizing clause.

---

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

4 Files Affected:

• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+45-12)
• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+9-3)
• (modified) flang/test/Lower/OpenMP/taskgroup02.f90 (+3-2)
• (modified) llvm/include/llvm/Frontend/OpenMP/OMP.h (+4)

diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 3fae3f3a0ddfd..675a58e4f35a1 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -26,6 +26,8 @@
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
 #include "flang/Semantics/attr.h"
 #include "flang/Semantics/tools.h"
+#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/SmallSet.h"
 
 namespace Fortran {
 namespace lower {
@@ -49,7 +51,7 @@ DataSharingProcessor::DataSharingProcessor(
       firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
       shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
       useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
-      visitor() {
+      visitor(semaCtx) {
   eval.visit([&](const auto &functionParserNode) {
     parser::Walk(functionParserNode, visitor);
   });
@@ -424,24 +426,55 @@ getSource(const semantics::SemanticsContext &semaCtx,
   return source;
 }
 
+static void collectPrivatizingConstructs(
+    llvm::SmallSet<llvm::omp::Directive, 16> &constructs, unsigned version) {
+  using Clause = llvm::omp::Clause;
+  using Directive = llvm::omp::Directive;
+
+  static const Clause privatizingClauses[] = {
+      Clause::OMPC_private,
+      Clause::OMPC_lastprivate,
+      Clause::OMPC_firstprivate,
+      Clause::OMPC_in_reduction,
+      Clause::OMPC_reduction,
+      Clause::OMPC_linear,
+      // TODO: Clause::OMPC_induction,
+      Clause::OMPC_task_reduction,
+      Clause::OMPC_detach,
+      Clause::OMPC_use_device_ptr,
+      Clause::OMPC_is_device_ptr,
+  };
+
+  for (auto dir : llvm::enum_seq_inclusive<Directive>(Directive::First_,
+                                                      Directive::Last_)) {
+    bool allowsPrivatizing = llvm::any_of(privatizingClauses, [&](Clause cls) {
+      return llvm::omp::isAllowedClauseForDirective(dir, cls, version);
+    });
+    if (allowsPrivatizing)
+      constructs.insert(dir);
+  }
+}
+
 bool DataSharingProcessor::isOpenMPPrivatizingConstruct(
-    const parser::OpenMPConstruct &omp) {
-  return common::visit(
-      [](auto &&s) {
-        using BareS = llvm::remove_cvref_t<decltype(s)>;
-        return std::is_same_v<BareS, parser::OpenMPBlockConstruct> ||
-               std::is_same_v<BareS, parser::OpenMPLoopConstruct> ||
-               std::is_same_v<BareS, parser::OpenMPSectionsConstruct>;
-      },
-      omp.u);
+    const parser::OpenMPConstruct &omp, unsigned version) {
+  static llvm::SmallSet<llvm::omp::Directive, 16> privatizing;
+  [[maybe_unused]] static bool init =
+      (collectPrivatizingConstructs(privatizing, version), true);
+
+  // As of OpenMP 6.0, privatizing constructs (with the test being if they
+  // allow a privatizing clause) are: dispatch, distribute, do, for, loop,
+  // parallel, scope, sections, simd, single, target, target_data, task,
+  // taskgroup, taskloop, and teams.
+  return llvm::is_contained(privatizing, extractOmpDirective(omp));
 }
 
 bool DataSharingProcessor::isOpenMPPrivatizingEvaluation(
     const pft::Evaluation &eval) const {
-  return eval.visit([](auto &&s) {
+  unsigned version = semaCtx.langOptions().OpenMPVersion;
+  return eval.visit([=](auto &&s) {
     using BareS = llvm::remove_cvref_t<decltype(s)>;
     if constexpr (std::is_same_v<BareS, parser::OpenMPConstruct>) {
-      return isOpenMPPrivatizingConstruct(s);
+      return isOpenMPPrivatizingConstruct(s, version);
     } else {
       return false;
     }
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index ee2fc70d2e673..bc422f410403a 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -36,6 +36,8 @@ class DataSharingProcessor {
   /// at any point in time. This is used to track Symbol definition scopes in
   /// order to tell which OMP scope defined vs. references a certain Symbol.
   struct OMPConstructSymbolVisitor {
+    OMPConstructSymbolVisitor(semantics::SemanticsContext &ctx)
+        : version(ctx.langOptions().OpenMPVersion) {}
     template <typename T>
     bool Pre(const T &) {
       return true;
@@ -45,13 +47,13 @@ class DataSharingProcessor {
 
     bool Pre(const parser::OpenMPConstruct &omp) {
       // Skip constructs that may not have privatizations.
-      if (isOpenMPPrivatizingConstruct(omp))
+      if (isOpenMPPrivatizingConstruct(omp, version))
         constructs.push_back(&omp);
       return true;
     }
 
     void Post(const parser::OpenMPConstruct &omp) {
-      if (isOpenMPPrivatizingConstruct(omp))
+      if (isOpenMPPrivatizingConstruct(omp, version))
         constructs.pop_back();
     }
 
@@ -68,6 +70,9 @@ class DataSharingProcessor {
     /// construct that defines symbol.
     bool isSymbolDefineBy(const semantics::Symbol *symbol,
                           lower::pft::Evaluation &eval) const;
+
+  private:
+    unsigned version;
   };
 
   mlir::OpBuilder::InsertPoint lastPrivIP;
@@ -115,7 +120,8 @@ class DataSharingProcessor {
                              mlir::OpBuilder::InsertPoint *lastPrivIP);
   void insertDeallocs();
 
-  static bool isOpenMPPrivatizingConstruct(const parser::OpenMPConstruct &omp);
+  static bool isOpenMPPrivatizingConstruct(const parser::OpenMPConstruct &omp,
+                                           unsigned version);
   bool isOpenMPPrivatizingEvaluation(const pft::Evaluation &eval) const;
 
 public:
diff --git a/flang/test/Lower/OpenMP/taskgroup02.f90 b/flang/test/Lower/OpenMP/taskgroup02.f90
index 1e996a030c23a..4c470b7aa82d1 100644
--- a/flang/test/Lower/OpenMP/taskgroup02.f90
+++ b/flang/test/Lower/OpenMP/taskgroup02.f90
@@ -3,8 +3,9 @@
 ! Check that variables are not privatized twice when TASKGROUP is used.
 
 !CHECK-LABEL: func.func @_QPsub() {
-!CHECK:         omp.parallel {
-!CHECK:           %[[PAR_I:.*]]:2 = hlfir.declare %{{.*}} {uniq_name = "_QFsubEi"}
+!CHECK:         omp.parallel private(@_QFsubEi_private_i32 %[[SUB_I:.*]]#0 -> %[[ARG:.*]] : !fir.ref<i32>)
+!CHECK:           %[[ALLOCA:.*]] = fir.alloca i32
+!CHECK:           %[[PAR_I:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "_QFsubEi"}
 !CHECK:           omp.master {
 !CHECK:             omp.taskgroup {
 !CHECK-NEXT:          omp.task private(@_QFsubEi_firstprivate_i32 %[[PAR_I]]#0 -> %[[TASK_I:.*]] : !fir.ref<i32>) {
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.h b/llvm/include/llvm/Frontend/OpenMP/OMP.h
index d44c33301bde7..9d0a55432e1ae 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.h
@@ -51,13 +51,17 @@ static constexpr inline bool canHaveIterator(Clause C) {
 // Can clause C create a private copy of a variable.
 static constexpr inline bool isPrivatizingClause(Clause C) {
   switch (C) {
+  case OMPC_detach:
   case OMPC_firstprivate:
+  // TODO case OMPC_induction:
   case OMPC_in_reduction:
+  case OMPC_is_device_ptr:
   case OMPC_lastprivate:
   case OMPC_linear:
   case OMPC_private:
   case OMPC_reduction:
   case OMPC_task_reduction:
+  case OMPC_use_device_ptr:
     return true;
   default:
     return false;

[kparzysz]

Stack:

• [Frontend][OpenMP] Move isPrivatizingClause to OMP.h, NFC #148644
• [utils][TableGen] Make some non-bitmask enums iterable #148647
• [flang][OpenMP] Move extractOmpDirective to Utils.cpp, NFC #148653
• [flang][OpenMP] Generalize isOpenMPPrivatizingConstruct #148654 (this PR)

[luporl]

In this test, was the issue the fact that i was not being privatized because taskgroup was not being considered as a privatizing construct, which made i not be collected as a symbol in nested regions?

Was i getting a private copy just by accident, because it was a loop-iteration variable inside parallel in this case?

[kparzysz]

We treated all block-associated constructs as privatizing, including master (which isn't one). When master is no longer considered as privatizing the result is that we privatize i in `parallel.

[luporl]

LGTM