Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang][OpenMP] Support tasks' implicit firstprivate DSA #85989

Merged
merged 1 commit into from
May 6, 2024

Conversation

luporl
Copy link
Contributor

@luporl luporl commented Mar 20, 2024

Handle implicit firstprivate DSAs on task generating constructs.

Fixes #64480

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics labels Mar 20, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 20, 2024

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

@llvm/pr-subscribers-flang-openmp

Author: Leandro Lupori (luporl)

Changes

Handle implicit firstprivate DSAs on task generating constructs.

Fixes #64480


Patch is 42.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85989.diff

10 Files Affected:

  • (modified) flang/include/flang/Semantics/symbol.h (+2-1)
  • (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+101-13)
  • (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+8-3)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+69-16)
  • (modified) flang/test/Lower/OpenMP/FIR/default-clause.f90 (+1-2)
  • (modified) flang/test/Lower/OpenMP/default-clause-byref.f90 (+1-3)
  • (modified) flang/test/Lower/OpenMP/default-clause.f90 (+1-3)
  • (added) flang/test/Lower/OpenMP/implicit-dsa.f90 (+275)
  • (added) flang/test/Semantics/OpenMP/implicit-dsa.f90 (+158)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+1-1)
diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
index 67153ffb3be9f6..34ac674614a695 100644
--- a/flang/include/flang/Semantics/symbol.h
+++ b/flang/include/flang/Semantics/symbol.h
@@ -740,7 +740,8 @@ class Symbol {
       OmpCommonBlock, OmpReduction, OmpAligned, OmpNontemporal, OmpAllocate,
       OmpDeclarativeAllocateDirective, OmpExecutableAllocateDirective,
       OmpDeclareSimd, OmpDeclareTarget, OmpThreadprivate, OmpDeclareReduction,
-      OmpFlushed, OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined);
+      OmpFlushed, OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined,
+      OmpImplicit);
   using Flags = common::EnumSet<Flag, Flag_enumSize>;
 
   const Scope &owner() const { return *owner_; }
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 90c7e46dd183e3..792b3341ef03cb 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -26,8 +26,10 @@ namespace omp {
 void DataSharingProcessor::processStep1() {
   collectSymbolsForPrivatization();
   collectDefaultSymbols();
+  collectImplicitSymbols();
   privatize();
   defaultPrivatize();
+  implicitPrivatize();
   insertBarrier();
 }
 
@@ -268,16 +270,94 @@ void DataSharingProcessor::insertLastPrivateCompare(mlir::Operation *op) {
   firOpBuilder.restoreInsertionPoint(localInsPt);
 }
 
+static const Fortran::parser::CharBlock *
+getSource(const Fortran::semantics::SemanticsContext &semaCtx,
+          const Fortran::lower::pft::Evaluation &eval) {
+  const Fortran::parser::CharBlock *source = nullptr;
+
+  auto ompConsVisit = [&](const Fortran::parser::OpenMPConstruct &x) {
+    std::visit(Fortran::common::visitors{
+                   [&](const Fortran::parser::OpenMPSectionsConstruct &x) {
+                     source = &std::get<0>(x.t).source;
+                   },
+                   [&](const Fortran::parser::OpenMPLoopConstruct &x) {
+                     source = &std::get<0>(x.t).source;
+                   },
+                   [&](const Fortran::parser::OpenMPBlockConstruct &x) {
+                     source = &std::get<0>(x.t).source;
+                   },
+                   [&](const Fortran::parser::OpenMPCriticalConstruct &x) {
+                     source = &std::get<0>(x.t).source;
+                   },
+                   [&](const Fortran::parser::OpenMPAtomicConstruct &x) {
+                     std::visit([&](const auto &x) { source = &x.source; },
+                                x.u);
+                   },
+                   [&](const auto &x) { source = &x.source; },
+               },
+               x.u);
+  };
+
+  eval.visit(Fortran::common::visitors{
+      [&](const Fortran::parser::OpenMPConstruct &x) { ompConsVisit(x); },
+      [&](const Fortran::parser::OpenMPDeclarativeConstruct &x) {
+        source = &x.source;
+      },
+      [&](const Fortran::parser::OmpEndLoopDirective &x) {
+        source = &x.source;
+      },
+      [&](const auto &x) {},
+  });
+
+  return source;
+}
+
 void DataSharingProcessor::collectSymbols(
-    Fortran::semantics::Symbol::Flag flag) {
-  converter.collectSymbolSet(eval, defaultSymbols, flag,
+    Fortran::semantics::Symbol::Flag flag,
+    llvm::SetVector<const Fortran::semantics::Symbol *> &symbols) {
+  // Collect all scopes associated with 'eval'.
+  llvm::SetVector<const Fortran::semantics::Scope *> clauseScopes;
+  std::function<void(const Fortran::semantics::Scope *)> collectScopes =
+      [&](const Fortran::semantics::Scope *scope) {
+        clauseScopes.insert(scope);
+        for (const Fortran::semantics::Scope &child : scope->children())
+          collectScopes(&child);
+      };
+  const Fortran::parser::CharBlock *source =
+      clauses.empty() ? getSource(semaCtx, eval) : &clauses.front().source;
+  const Fortran::semantics::Scope *curScope = nullptr;
+  if (source && !source->empty()) {
+    curScope = &semaCtx.FindScope(*source);
+    collectScopes(curScope);
+  }
+  // Collect all symbols referenced in the evaluation being processed,
+  // that matches 'flag'.
+  llvm::SetVector<const Fortran::semantics::Symbol *> allSymbols;
+  converter.collectSymbolSet(eval, allSymbols, flag,
                              /*collectSymbols=*/true,
                              /*collectHostAssociatedSymbols=*/true);
-  for (Fortran::lower::pft::Evaluation &e : eval.getNestedEvaluations()) {
+  llvm::SetVector<const Fortran::semantics::Symbol *> symbolsInNestedRegions;
+  for (Fortran::lower::pft::Evaluation &e : eval.getNestedEvaluations())
     if (e.hasNestedEvaluations() && !e.isConstruct())
       converter.collectSymbolSet(e, symbolsInNestedRegions, flag,
                                  /*collectSymbols=*/true,
                                  /*collectHostAssociatedSymbols=*/false);
+  // Filter-out symbols that must not be privatized.
+  bool collectImplicit = flag == Fortran::semantics::Symbol::Flag::OmpImplicit;
+  auto isPrivatizable = [](const Fortran::semantics::Symbol &sym) -> bool {
+    return !Fortran::semantics::IsProcedure(sym) &&
+           !sym.GetUltimate().has<Fortran::semantics::DerivedTypeDetails>() &&
+           !sym.GetUltimate().has<Fortran::semantics::NamelistDetails>();
+  };
+  for (const auto *sym : allSymbols) {
+    assert(curScope && "couldn't find current scope");
+    if (isPrivatizable(*sym) && !symbolsInNestedRegions.contains(sym) &&
+        !privatizedSymbols.contains(sym) &&
+        !sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined) &&
+        (collectImplicit ||
+         !sym->test(Fortran::semantics::Symbol::Flag::OmpImplicit)) &&
+        clauseScopes.contains(&sym->owner()))
+      symbols.insert(sym);
   }
 }
 
@@ -286,13 +366,22 @@ void DataSharingProcessor::collectDefaultSymbols() {
     if (const auto *defaultClause =
             std::get_if<omp::clause::Default>(&clause.u)) {
       if (defaultClause->v == omp::clause::Default::Type::Private)
-        collectSymbols(Fortran::semantics::Symbol::Flag::OmpPrivate);
+        collectSymbols(Fortran::semantics::Symbol::Flag::OmpPrivate,
+                       defaultSymbols);
       else if (defaultClause->v == omp::clause::Default::Type::Firstprivate)
-        collectSymbols(Fortran::semantics::Symbol::Flag::OmpFirstPrivate);
+        collectSymbols(Fortran::semantics::Symbol::Flag::OmpFirstPrivate,
+                       defaultSymbols);
     }
   }
 }
 
+void DataSharingProcessor::collectImplicitSymbols() {
+  // There will be no implicit symbols when a default clause is present.
+  if (defaultSymbols.empty())
+    collectSymbols(Fortran::semantics::Symbol::Flag::OmpImplicit,
+                   implicitSymbols);
+}
+
 void DataSharingProcessor::privatize() {
   for (const Fortran::semantics::Symbol *sym : privatizedSymbols) {
     if (const auto *commonDet =
@@ -318,14 +407,13 @@ void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
 }
 
 void DataSharingProcessor::defaultPrivatize() {
-  for (const Fortran::semantics::Symbol *sym : defaultSymbols) {
-    if (!Fortran::semantics::IsProcedure(*sym) &&
-        !sym->GetUltimate().has<Fortran::semantics::DerivedTypeDetails>() &&
-        !sym->GetUltimate().has<Fortran::semantics::NamelistDetails>() &&
-        !symbolsInNestedRegions.contains(sym) &&
-        !privatizedSymbols.contains(sym))
-      doPrivatize(sym);
-  }
+  for (const Fortran::semantics::Symbol *sym : defaultSymbols)
+    doPrivatize(sym);
+}
+
+void DataSharingProcessor::implicitPrivatize() {
+  for (const Fortran::semantics::Symbol *sym : implicitSymbols)
+    doPrivatize(sym);
 }
 
 void DataSharingProcessor::doPrivatize(const Fortran::semantics::Symbol *sym) {
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 002bb119c0b916..e5b695f3f1e77f 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -49,8 +49,9 @@ class DataSharingProcessor {
   // Symbols in private, firstprivate, and/or lastprivate clauses.
   llvm::SetVector<const Fortran::semantics::Symbol *> privatizedSymbols;
   llvm::SetVector<const Fortran::semantics::Symbol *> defaultSymbols;
-  llvm::SetVector<const Fortran::semantics::Symbol *> symbolsInNestedRegions;
+  llvm::SetVector<const Fortran::semantics::Symbol *> implicitSymbols;
   Fortran::lower::AbstractConverter &converter;
+  Fortran::semantics::SemanticsContext &semaCtx;
   fir::FirOpBuilder &firOpBuilder;
   omp::List<omp::Clause> clauses;
   Fortran::lower::pft::Evaluation &eval;
@@ -59,15 +60,19 @@ class DataSharingProcessor {
   DelayedPrivatizationInfo delayedPrivatizationInfo;
 
   bool needBarrier();
-  void collectSymbols(Fortran::semantics::Symbol::Flag flag);
+  void
+  collectSymbols(Fortran::semantics::Symbol::Flag flag,
+                 llvm::SetVector<const Fortran::semantics::Symbol *> &symbols);
   void collectOmpObjectListSymbol(
       const omp::ObjectList &objects,
       llvm::SetVector<const Fortran::semantics::Symbol *> &symbolSet);
   void collectSymbolsForPrivatization();
   void insertBarrier();
   void collectDefaultSymbols();
+  void collectImplicitSymbols();
   void privatize();
   void defaultPrivatize();
+  void implicitPrivatize();
   void doPrivatize(const Fortran::semantics::Symbol *sym);
   void copyLastPrivatize(mlir::Operation *op);
   void insertLastPrivateCompare(mlir::Operation *op);
@@ -86,7 +91,7 @@ class DataSharingProcessor {
                        Fortran::lower::pft::Evaluation &eval,
                        bool useDelayedPrivatization = false,
                        Fortran::lower::SymMap *symTable = nullptr)
-      : hasLastPrivateOp(false), converter(converter),
+      : hasLastPrivateOp(false), converter(converter), semaCtx(semaCtx),
         firOpBuilder(converter.getFirOpBuilder()),
         clauses(omp::makeList(opClauseList, semaCtx)), eval(eval),
         useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 6d58013b87d298..57ba556b9d57bf 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2012,34 +2012,87 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
         }
       }
     }
-    std::vector<Symbol *> defaultDSASymbols;
+
+    // Implicitly determined DSAs
+    // OMP 5.2 5.1.1 - Variables Referenced in a Construct
+    Symbol *lastDeclSymbol = nullptr;
+    std::optional<Symbol::Flag> prevDSA;
     for (int dirDepth{0}; dirDepth < (int)dirContext_.size(); ++dirDepth) {
       DirContext &dirContext = dirContext_[dirDepth];
-      bool hasDataSharingAttr{false};
+      std::optional<Symbol::Flag> dsa;
+
       for (auto symMap : dirContext.objectWithDSA) {
         // if the `symbol` already has a data-sharing attribute
         if (symMap.first->name() == name.symbol->name()) {
-          hasDataSharingAttr = true;
+          dsa = symMap.second;
           break;
         }
       }
-      if (hasDataSharingAttr) {
-        if (defaultDSASymbols.size())
-          symbol = &MakeAssocSymbol(symbol->name(), *defaultDSASymbols.back(),
+
+      // When handling each implicit rule, either a new private symbol is
+      // declared or the last declared symbol is used.
+      // In the latter case, it's necessary to insert a new symbol in the scope
+      // being processed, associated with the last declared symbol, to avoid
+      // "inheriting" the enclosing context's symbol and its flags.
+      auto declNewSymbol = [&](Symbol::Flag flag) {
+        Symbol *hostSymbol =
+            lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
+        lastDeclSymbol = DeclarePrivateAccessEntity(
+            *hostSymbol, flag, context_.FindScope(dirContext.directiveSource));
+        return lastDeclSymbol;
+      };
+      auto useLastDeclSymbol = [&]() {
+        if (lastDeclSymbol)
+          MakeAssocSymbol(symbol->name(), *lastDeclSymbol,
               context_.FindScope(dirContext.directiveSource));
+      };
+
+      if (dsa.has_value()) {
+        useLastDeclSymbol();
+        prevDSA = dsa;
         continue;
       }
 
-      if (dirContext.defaultDSA == semantics::Symbol::Flag::OmpPrivate ||
-          dirContext.defaultDSA == semantics::Symbol::Flag::OmpFirstPrivate) {
-        Symbol *hostSymbol = defaultDSASymbols.size() ? defaultDSASymbols.back()
-                                                      : &symbol->GetUltimate();
-        defaultDSASymbols.push_back(
-            DeclarePrivateAccessEntity(*hostSymbol, dirContext.defaultDSA,
-                context_.FindScope(dirContext.directiveSource)));
-      } else if (defaultDSASymbols.size())
-        symbol = &MakeAssocSymbol(symbol->name(), *defaultDSASymbols.back(),
-            context_.FindScope(dirContext.directiveSource));
+      bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
+      bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
+      bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
+
+      if (dirContext.defaultDSA == Symbol::Flag::OmpPrivate ||
+          dirContext.defaultDSA == Symbol::Flag::OmpFirstPrivate ||
+          dirContext.defaultDSA == Symbol::Flag::OmpShared) {
+        // 1) default
+        // Allowed only with parallel, teams and task generating constructs.
+        assert(parallelDir || taskGenDir ||
+            llvm::omp::allTeamsSet.test(dirContext.directive));
+        if (dirContext.defaultDSA != Symbol::Flag::OmpShared)
+          declNewSymbol(dirContext.defaultDSA);
+        else
+          useLastDeclSymbol();
+        dsa = dirContext.defaultDSA;
+      } else if (parallelDir) {
+        // 2) parallel -> shared
+        useLastDeclSymbol();
+        dsa = Symbol::Flag::OmpShared;
+      } else if (!taskGenDir && !targetDir) {
+        // 3) enclosing context
+        useLastDeclSymbol();
+        dsa = prevDSA;
+      } else if (targetDir) {
+        // TODO 4) not mapped target variable -> firstprivate
+        dsa = prevDSA;
+      } else if (taskGenDir) {
+        // TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
+        if (prevDSA == Symbol::Flag::OmpShared) {
+          // 6) shared in enclosing context -> shared
+          useLastDeclSymbol();
+          dsa = Symbol::Flag::OmpShared;
+        } else {
+          // 7) firstprivate
+          dsa = Symbol::Flag::OmpFirstPrivate;
+          declNewSymbol(*dsa)->set(Symbol::Flag::OmpImplicit);
+        }
+      }
+      prevDSA = dsa;
     }
   } // within OpenMP construct
 }
diff --git a/flang/test/Lower/OpenMP/FIR/default-clause.f90 b/flang/test/Lower/OpenMP/FIR/default-clause.f90
index 8d131c5d69b16a..bc825f0e9dba3f 100644
--- a/flang/test/Lower/OpenMP/FIR/default-clause.f90
+++ b/flang/test/Lower/OpenMP/FIR/default-clause.f90
@@ -213,11 +213,10 @@ subroutine nested_default_clause_tests
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.parallel {
-!CHECK: %[[PRIVATE_INNER_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
 !CHECK: %[[PRIVATE_INNER_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"} 
 !CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
 !CHECK: %[[temp_1:.*]] = fir.load %[[PRIVATE_INNER_X]] : !fir.ref<i32>
-!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_INNER_Z]] : !fir.ref<i32>
+!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_Z]] : !fir.ref<i32>
 !CHECK: %[[result:.*]] = arith.addi %[[temp_1]], %[[temp_2]] : i32
 !CHECK: fir.store %[[result]] to %[[PRIVATE_INNER_W]] : !fir.ref<i32>
 !CHECK: omp.terminator
diff --git a/flang/test/Lower/OpenMP/default-clause-byref.f90 b/flang/test/Lower/OpenMP/default-clause-byref.f90
index 86da354910a8ef..f0a117b4c6c2f8 100644
--- a/flang/test/Lower/OpenMP/default-clause-byref.f90
+++ b/flang/test/Lower/OpenMP/default-clause-byref.f90
@@ -240,14 +240,12 @@ subroutine nested_default_clause_tests
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.parallel {
-!CHECK: %[[PRIVATE_INNER_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_testsEz"}
-!CHECK: %[[PRIVATE_INNER_Z_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_Z]] {uniq_name = "_QFnested_default_clause_testsEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_INNER_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"}
 !CHECK: %[[PRIVATE_INNER_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_W]] {uniq_name = "_QFnested_default_clause_testsEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
 !CHECK: %[[PRIVATE_INNER_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_X]] {uniq_name = "_QFnested_default_clause_testsEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP_1:.*]] = fir.load %[[PRIVATE_INNER_X_DECL]]#0 : !fir.ref<i32>
-!CHECK: %[[TEMP_2:.*]] = fir.load %[[PRIVATE_INNER_Z_DECL]]#0 : !fir.ref<i32>
+!CHECK: %[[TEMP_2:.*]] = fir.load %[[PRIVATE_Z_DECL]]#0 : !fir.ref<i32>
 !CHECK: %[[RESULT:.*]] = arith.addi %{{.*}}, %{{.*}} : i32
 !CHECK: hlfir.assign %[[RESULT]] to %[[PRIVATE_INNER_W_DECL]]#0 : i32, !fir.ref<i32>
 !CHECK: omp.terminator
diff --git a/flang/test/Lower/OpenMP/default-clause.f90 b/flang/test/Lower/OpenMP/default-clause.f90
index f69b5e607d3561..c4e6c7532026c7 100644
--- a/flang/test/Lower/OpenMP/default-clause.f90
+++ b/flang/test/Lower/OpenMP/default-clause.f90
@@ -254,14 +254,12 @@ subroutine nested_default_clause_test1
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.parallel {
-!CHECK: %[[PRIVATE_INNER_Z:.*]] = fir.alloca i32 {bindc_name = "z", pinned, uniq_name = "_QFnested_default_clause_test2Ez"}
-!CHECK: %[[PRIVATE_INNER_Z_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_Z]] {uniq_name = "_QFnested_default_clause_test2Ez"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_INNER_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_test2Ew"}
 !CHECK: %[[PRIVATE_INNER_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_W]] {uniq_name = "_QFnested_default_clause_test2Ew"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_test2Ex"}
 !CHECK: %[[PRIVATE_INNER_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_INNER_X]] {uniq_name = "_QFnested_default_clause_test2Ex"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP_1:.*]] = fir.load %[[PRIVATE_INNER_X_DECL]]#0 : !fir.ref<i32>
-!CHECK: %[[TEMP_2:.*]] = fir.load %[[PRIVATE_INNER_Z_DECL]]#0 : !fir.ref<i32>
+!CHECK: %[[TEMP_2:.*]] = fir.load %[[PRIVATE_Z_DECL]]#0 : !fir.ref<i32>
 !CHECK: %[[RESULT:.*]] = arith.addi %{{.*}}, %{{.*}} : i32
 !CHECK: hlfir.assign %[[RESULT]] to %[[PRIVATE_INNER_W_DECL]]#0 : i32, !fir.ref<i32>
 !CHECK: omp.terminator
diff --git a/flang/test/Lower/OpenMP/implicit-dsa.f90 b/flang/test/Lower/OpenMP/implicit-dsa.f90
new file mode 100644
index 00000000000000..0f67d5bfd194f9
--- /dev/null
+++ b/flang/test/Lower/OpenMP/implicit-dsa.f90
@@ -0,0 +1,275 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+! Checks lowering of OpenMP variables with implicitly determined DSAs.
+
+! Basic cases.
+!CHECK-LABEL: func @_QPimplicit_dsa_test1
+!CHECK:       %[[X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFimplicit_dsa_test1Ex"}
+!CHECK:       %[[X_DECL:.*]]:2 = hlfir.declare %[[X]] {uniq_name = "_QFimplicit_dsa_test1Ex"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:       %[[Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFimplicit_dsa_test1Ey"}
+!CHECK:       %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y]] {uniq_name = "_QFimplicit_dsa_test1Ey"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:       %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFimplicit_dsa_test1Ez"}
+!CHECK:       %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFimplicit_dsa_test1Ez"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:       omp.task ...
[truncated]

@kiranchandramohan
Copy link
Contributor

I am away this week, will come back to this next week.

Copy link
Contributor

@NimishMishra NimishMishra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for these changes. I went through the changes; the handling of symbols in semantics seems particularly fine.

LGTM! The windows builtbot is failing, though Linux is passing.

!CHECK: %[[PRIVATE_INNER_W:.*]] = fir.alloca i32 {bindc_name = "w", pinned, uniq_name = "_QFnested_default_clause_testsEw"}
!CHECK: %[[PRIVATE_INNER_X:.*]] = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFnested_default_clause_testsEx"}
!CHECK: %[[temp_1:.*]] = fir.load %[[PRIVATE_INNER_X]] : !fir.ref<i32>
!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_INNER_Z]] : !fir.ref<i32>
!CHECK: %[[temp_2:.*]] = fir.load %[[PRIVATE_Z]] : !fir.ref<i32>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Indeed shared(z) needs to have PRIVATE_Z and not PRIVATE_INNER_Z.

@NimishMishra
Copy link
Contributor

Also, the PR shows "[luporl] wants to merge 1 commit into [llvm:users/luporl/refactor-default-test]from [luporl:luporl-omp-implicit]"

Is this merge not in main?

@luporl
Copy link
Contributor Author

luporl commented Mar 26, 2024

@NimishMishra Thanks for the review!

It seems the buildbot errors on Windows are all of this type: fatal error C1002: compiler is out of heap space in pass 2.

Is this merge not in main?

It is not. It depends on #85978, that depends on #72510. As tasks' implicit firstprivate happens only when no default clause is specified, I thought it would be better to start from the default clause fix, in #72510.

Actually, #78283 is also needed to avoid issues with threadprivate.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG. Nice work.

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
@luporl luporl changed the base branch from users/luporl/refactor-default-test to main May 3, 2024 12:59
Handle implicit firstprivate DSAs on task generating constructs.

Fixes llvm#64480
@luporl
Copy link
Contributor Author

luporl commented May 6, 2024

Windows' CI error seems unrelated:
llvm\lib\Target\AMDGPU\SIModeRegisterDefaults.cpp(239): error C2131: expression did not evaluate to a constant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:openmp flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flang][OpenMP]Variable used in TASK construct is not implicit firstprivate
4 participants