Reapply #91116 with fix

flang/include/flang/Lower/AbstractConverter.h

@@ -131,7 +131,8 @@ class AbstractConverter {
 
   /// For a given symbol, check if it is present in the inner-most
   /// level of the symbol map.
-  virtual bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) = 0;
+  virtual bool
+  isPresentShallowLookup(const Fortran::semantics::Symbol &sym) = 0;
 
   /// Collect the set of symbols with \p flag in \p eval
   /// region if \p collectSymbols is true. Otherwise, collect the

flang/lib/Lower/Bridge.cpp

@@ -602,7 +602,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
     return typeConstructionStack;
   }
 
-  bool isPresentShallowLookup(Fortran::semantics::Symbol &sym) override final {
+  bool
+  isPresentShallowLookup(const Fortran::semantics::Symbol &sym) override final {
     return bool(shallowLookupSymbol(sym));
   }
 

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

@@ -22,16 +22,41 @@
 namespace Fortran {
 namespace lower {
 namespace omp {
+bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
+    const semantics::Symbol *symbol, lower::pft::Evaluation &eval) const {
+  return eval.visit(
+      common::visitors{[&](const parser::OpenMPConstruct &functionParserNode) {
+                         return symDefMap.count(symbol) &&
+                                symDefMap.at(symbol) == &functionParserNode;
+                       },
+                       [](const auto &functionParserNode) { return false; }});
+}
+
+DataSharingProcessor::DataSharingProcessor(
+    lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
+    const List<Clause> &clauses, lower::pft::Evaluation &eval,
+    bool shouldCollectPreDeterminedSymbols, bool useDelayedPrivatization,
+    lower::SymMap *symTable)
+    : hasLastPrivateOp(false), converter(converter), semaCtx(semaCtx),
+      firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
+      shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
+      useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
+      visitor() {
+  eval.visit([&](const auto &functionParserNode) {
+    parser::Walk(functionParserNode, visitor);
+  });
+}
 
 void DataSharingProcessor::processStep1(
     mlir::omp::PrivateClauseOps *clauseOps,
     llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
   collectSymbolsForPrivatization();
   collectDefaultSymbols();
   collectImplicitSymbols();
+  collectPreDeterminedSymbols();
+
   privatize(clauseOps, privateSyms);
-  defaultPrivatize(clauseOps, privateSyms);
-  implicitPrivatize(clauseOps, privateSyms);
+
   insertBarrier();
 }
 
@@ -57,7 +82,7 @@ void DataSharingProcessor::processStep2(mlir::Operation *op, bool isLoop) {
 }
 
 void DataSharingProcessor::insertDeallocs() {
-  for (const semantics::Symbol *sym : privatizedSymbols)
+  for (const semantics::Symbol *sym : allPrivatizedSymbols)
     if (semantics::IsAllocatable(sym->GetUltimate())) {
       if (!useDelayedPrivatization) {
         converter.createHostAssociateVarCloneDealloc(*sym);
@@ -92,10 +117,6 @@ void DataSharingProcessor::insertDeallocs() {
 }
 
 void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) {
-  // Privatization for symbols which are pre-determined (like loop index
-  // variables) happen separately, for everything else privatize here.
-  if (sym->test(semantics::Symbol::Flag::OmpPreDetermined))
-    return;
   bool success = converter.createHostAssociateVarClone(*sym);
   (void)success;
   assert(success && "Privatization failed due to existing binding");
@@ -126,20 +147,24 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
   for (const omp::Clause &clause : clauses) {
     if (const auto &privateClause =
             std::get_if<omp::clause::Private>(&clause.u)) {
-      collectOmpObjectListSymbol(privateClause->v, privatizedSymbols);
+      collectOmpObjectListSymbol(privateClause->v, explicitlyPrivatizedSymbols);
     } else if (const auto &firstPrivateClause =
                    std::get_if<omp::clause::Firstprivate>(&clause.u)) {
-      collectOmpObjectListSymbol(firstPrivateClause->v, privatizedSymbols);
+      collectOmpObjectListSymbol(firstPrivateClause->v,
+                                 explicitlyPrivatizedSymbols);
     } else if (const auto &lastPrivateClause =
                    std::get_if<omp::clause::Lastprivate>(&clause.u)) {
       const ObjectList &objects = std::get<ObjectList>(lastPrivateClause->t);
-      collectOmpObjectListSymbol(objects, privatizedSymbols);
+      collectOmpObjectListSymbol(objects, explicitlyPrivatizedSymbols);
       hasLastPrivateOp = true;
     } else if (std::get_if<omp::clause::Collapse>(&clause.u)) {
       hasCollapse = true;
     }
   }
 
+  for (auto *sym : explicitlyPrivatizedSymbols)
+    allPrivatizedSymbols.insert(sym);
+
   if (hasCollapse && hasLastPrivateOp)
     TODO(converter.getCurrentLocation(), "Collapse clause with lastprivate");
 }
@@ -149,7 +174,7 @@ bool DataSharingProcessor::needBarrier() {
   // initialization of firstprivate variables and post-update of lastprivate
   // variables.
   // Emit implicit barrier for linear clause. Maybe on somewhere else.
-  for (const semantics::Symbol *sym : privatizedSymbols) {
+  for (const semantics::Symbol *sym : allPrivatizedSymbols) {
     if (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) &&
         sym->test(semantics::Symbol::Flag::OmpLastPrivate))
       return true;
@@ -283,10 +308,11 @@ void DataSharingProcessor::collectSymbolsInNestedRegions(
       if (nestedEval.isConstruct())
         // Recursively look for OpenMP constructs within `nestedEval`'s region
         collectSymbolsInNestedRegions(nestedEval, flag, symbolsInNestedRegions);
-      else
+      else {
         converter.collectSymbolSet(nestedEval, symbolsInNestedRegions, flag,
                                    /*collectSymbols=*/true,
                                    /*collectHostAssociatedSymbols=*/false);
+      }
     }
   }
 }
@@ -322,24 +348,44 @@ void DataSharingProcessor::collectSymbols(
   converter.collectSymbolSet(eval, allSymbols, flag,
                              /*collectSymbols=*/true,
                              /*collectHostAssociatedSymbols=*/true);
+
   llvm::SetVector<const semantics::Symbol *> symbolsInNestedRegions;
   collectSymbolsInNestedRegions(eval, flag, symbolsInNestedRegions);
+
+  for (auto *symbol : allSymbols)
+    if (visitor.isSymbolDefineBy(symbol, eval))
+      symbolsInNestedRegions.remove(symbol);
+
   // Filter-out symbols that must not be privatized.
   bool collectImplicit = flag == semantics::Symbol::Flag::OmpImplicit;
+  bool collectPreDetermined = flag == semantics::Symbol::Flag::OmpPreDetermined;
+
   auto isPrivatizable = [](const semantics::Symbol &sym) -> bool {
     return !semantics::IsProcedure(sym) &&
            !sym.GetUltimate().has<semantics::DerivedTypeDetails>() &&
            !sym.GetUltimate().has<semantics::NamelistDetails>() &&
            !semantics::IsImpliedDoIndex(sym.GetUltimate());
   };
+
+  auto shouldCollectSymbol = [&](const semantics::Symbol *sym) {
+    if (collectImplicit)
+      return sym->test(semantics::Symbol::Flag::OmpImplicit);
+
+    if (collectPreDetermined)
+      return sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+
+    return !sym->test(semantics::Symbol::Flag::OmpImplicit) &&
+           !sym->test(semantics::Symbol::Flag::OmpPreDetermined);
+  };
+
   for (const auto *sym : allSymbols) {
     assert(curScope && "couldn't find current scope");
     if (isPrivatizable(*sym) && !symbolsInNestedRegions.contains(sym) &&
-        !privatizedSymbols.contains(sym) &&
-        !sym->test(semantics::Symbol::Flag::OmpPreDetermined) &&
-        (collectImplicit || !sym->test(semantics::Symbol::Flag::OmpImplicit)) &&
-        clauseScopes.contains(&sym->owner()))
+        !explicitlyPrivatizedSymbols.contains(sym) &&
+        shouldCollectSymbol(sym) && clauseScopes.contains(&sym->owner())) {
+      allPrivatizedSymbols.insert(sym);
       symbols.insert(sym);
+    }
   }
 }
 
@@ -363,10 +409,16 @@ void DataSharingProcessor::collectImplicitSymbols() {
     collectSymbols(semantics::Symbol::Flag::OmpImplicit, implicitSymbols);
 }
 
+void DataSharingProcessor::collectPreDeterminedSymbols() {
+  if (shouldCollectPreDeterminedSymbols)
+    collectSymbols(semantics::Symbol::Flag::OmpPreDetermined,
+                   preDeterminedSymbols);
+}
+
 void DataSharingProcessor::privatize(
     mlir::omp::PrivateClauseOps *clauseOps,
     llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
-  for (const semantics::Symbol *sym : privatizedSymbols) {
+  for (const semantics::Symbol *sym : allPrivatizedSymbols) {
     if (const auto *commonDet =
             sym->detailsIf<semantics::CommonBlockDetails>()) {
       for (const auto &mem : commonDet->objects())
@@ -378,7 +430,7 @@ void DataSharingProcessor::privatize(
 
 void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
   insertLastPrivateCompare(op);
-  for (const semantics::Symbol *sym : privatizedSymbols)
+  for (const semantics::Symbol *sym : allPrivatizedSymbols)
     if (const auto *commonDet =
             sym->detailsIf<semantics::CommonBlockDetails>()) {
       for (const auto &mem : commonDet->objects()) {
@@ -389,20 +441,6 @@ void DataSharingProcessor::copyLastPrivatize(mlir::Operation *op) {
     }
 }
 
-void DataSharingProcessor::defaultPrivatize(
-    mlir::omp::PrivateClauseOps *clauseOps,
-    llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
-  for (const semantics::Symbol *sym : defaultSymbols)
-    doPrivatize(sym, clauseOps, privateSyms);
-}
-
-void DataSharingProcessor::implicitPrivatize(
-    mlir::omp::PrivateClauseOps *clauseOps,
-    llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {
-  for (const semantics::Symbol *sym : implicitSymbols)
-    doPrivatize(sym, clauseOps, privateSyms);
-}
-
 void DataSharingProcessor::doPrivatize(
     const semantics::Symbol *sym, mlir::omp::PrivateClauseOps *clauseOps,
     llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms) {

flang/lib/Lower/OpenMP/DataSharingProcessor.h

@@ -32,23 +32,62 @@ namespace omp {
 
 class DataSharingProcessor {
 private:
+  /// A symbol visitor that keeps track of the currently active OpenMPConstruct
+  /// 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 {
+    template <typename T>
+    bool Pre(const T &) {
+      return true;
+    }
+    template <typename T>
+    void Post(const T &) {}
+
+    bool Pre(const parser::OpenMPConstruct &omp) {
+      currentConstruct = &omp;
+      return true;
+    }
+
+    void Post(const parser::OpenMPConstruct &omp) {
+      currentConstruct = nullptr;
+    }
+
+    void Post(const parser::Name &name) {
+      symDefMap.try_emplace(name.symbol, currentConstruct);
+    }
+
+    const parser::OpenMPConstruct *currentConstruct = nullptr;
+    llvm::DenseMap<semantics::Symbol *, const parser::OpenMPConstruct *>
+        symDefMap;
+
+    /// Given a \p symbol and an \p eval, returns true if eval is the OMP
+    /// construct that defines symbol.
+    bool isSymbolDefineBy(const semantics::Symbol *symbol,
+                          lower::pft::Evaluation &eval) const;
+  };
+
   bool hasLastPrivateOp;
   mlir::OpBuilder::InsertPoint lastPrivIP;
   mlir::OpBuilder::InsertPoint insPt;
   mlir::Value loopIV;
   // Symbols in private, firstprivate, and/or lastprivate clauses.
-  llvm::SetVector<const semantics::Symbol *> privatizedSymbols;
+  llvm::SetVector<const semantics::Symbol *> explicitlyPrivatizedSymbols;
   llvm::SetVector<const semantics::Symbol *> defaultSymbols;
   llvm::SetVector<const semantics::Symbol *> implicitSymbols;
+  llvm::SetVector<const semantics::Symbol *> preDeterminedSymbols;
+  llvm::SetVector<const semantics::Symbol *> allPrivatizedSymbols;
+
   llvm::DenseMap<const semantics::Symbol *, mlir::omp::PrivateClauseOp>
       symToPrivatizer;
   lower::AbstractConverter &converter;
   semantics::SemanticsContext &semaCtx;
   fir::FirOpBuilder &firOpBuilder;
   omp::List<omp::Clause> clauses;
   lower::pft::Evaluation &eval;
+  bool shouldCollectPreDeterminedSymbols;
   bool useDelayedPrivatization;
   lower::SymMap *symTable;
+  OMPConstructSymbolVisitor visitor;
 
   bool needBarrier();
   void collectSymbols(semantics::Symbol::Flag flag,
@@ -63,6 +102,7 @@ class DataSharingProcessor {
   void insertBarrier();
   void collectDefaultSymbols();
   void collectImplicitSymbols();
+  void collectPreDeterminedSymbols();
   void privatize(mlir::omp::PrivateClauseOps *clauseOps,
                  llvm::SmallVectorImpl<const semantics::Symbol *> *privateSyms);
   void defaultPrivatize(
@@ -90,11 +130,9 @@ class DataSharingProcessor {
                        semantics::SemanticsContext &semaCtx,
                        const List<Clause> &clauses,
                        lower::pft::Evaluation &eval,
+                       bool shouldCollectPreDeterminedSymbols,
                        bool useDelayedPrivatization = false,
-                       lower::SymMap *symTable = nullptr)
-      : hasLastPrivateOp(false), converter(converter), semaCtx(semaCtx),
-        firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
-        useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
+                       lower::SymMap *symTable = nullptr);
 
   // Privatisation is split into two steps.
   // Step1 performs cloning of all privatisation clauses and copying for

flang/lib/Lower/OpenMP/Decomposer.cpp

@@ -123,4 +123,9 @@ ConstructQueue buildConstructQueue(
 
   return constructs;
 }
+
+bool isLastItemInQueue(ConstructQueue::iterator item,
+                       const ConstructQueue &queue) {
+  return std::next(item) == queue.end();
+}
 } // namespace Fortran::lower::omp

flang/lib/Lower/OpenMP/Decomposer.h

@@ -46,6 +46,9 @@ ConstructQueue buildConstructQueue(mlir::ModuleOp modOp,
                                    const parser::CharBlock &source,
                                    llvm::omp::Directive compound,
                                    const List<Clause> &clauses);
+
+bool isLastItemInQueue(ConstructQueue::iterator item,
+                       const ConstructQueue &queue);
 } // namespace Fortran::lower::omp
 
 #endif // FORTRAN_LOWER_OPENMP_DECOMPOSER_H