Skip to content

[flang][OpenMP] Explicitly set Shared DSA in symbols #142154

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 3, 2025
Merged

[flang][OpenMP] Explicitly set Shared DSA in symbols #142154

merged 2 commits into from
Jun 3, 2025

Conversation

luporl
Copy link
Contributor

@luporl luporl commented May 30, 2025 •
edited
Loading

Before this change, OmpShared was not always set in shared symbols.
Instead, absence of private flags was interpreted as shared DSA.
The problem was that symbols with no flags, with only a host
association, could also mean "has same DSA as in the enclosing
context". Now shared symbols behave the same as private and can be
treated the same way.

Because of the host association symbols with no flags mentioned
above, it was also incorrect to simply test the flags of a given
symbol to find out if it was private or shared. The function
GetSymbolDSA() was added to fix this. It would be better to avoid
the need of these special symbols, but this would require changes
to how symbols are collected in lowering.

Besides that, some semantic checks need to know if a DSA clause
was used or not. To avoid confusing implicit symbols with DSA
clauses a new flag was added: OmpExplicit. It is now set for all
symbols with explicitly determined data-sharing attributes.

With the changes above, AddToContextObjectWithDSA() and the symbol
to DSA map could probably be removed and the DSA could be obtained
directly from the symbol, but this was not attempted.

Some debug messages were also added, with the "omp" DEBUG_TYPE, to
make it easier to debug the creation of implicit symbols and to
visualize all associations of a given symbol.

Fixes #130533
Fixes #140882

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics labels May 30, 2025
@llvmbot
Copy link
Member

llvmbot commented May 30, 2025 •
edited
Loading

@llvm/pr-subscribers-flang-semantics

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

Author: Leandro Lupori (luporl)

Changes

Before this change, OmpShared was not always set in shared symbols.
Instead, absence of private flags was interpreted as shared DSA.
The problem was that symbols with no flags, with only a host
association, could also mean "has same DSA as in the enclosing
context". Now shared symbols behave the same as private and can be
treated the same way.

Because of the host association symbols with no flags mentioned
above, it was also incorrect to simply test the flags of a given
symbol to find out if it was private or shared. The function
GetSymbolDSA() was added to fix this. It would be better to avoid
the need of these special symbols, but this would require changes
to how symbols are collected in lowering.

Besides that, some semantic checks need to know if a DSA clause
was used or not. To avoid confusing implicit symbols with DSA
clauses a new flag was added: OmpExplicit. It is now set for all
symbols with explicitly determined data-sharing attributes.

With the changes above, AddToContextObjectWithDSA() and the symbol
to DSA map could probably be removed and the DSA could be obtained
directly from the symbol, but this was not attempted.

Some debug messages were also added, with the "omp" DEBUG_TYPE, to
make it easier to debug the creation of implicit symbols and to
visualize all associations of a given symbol.

Fixes #130533

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

26 Files Affected:

  • (added) flang/include/flang/Semantics/openmp-dsa.h (+20)
  • (modified) flang/include/flang/Semantics/symbol.h (+2-2)
  • (modified) flang/lib/Lower/Bridge.cpp (+3-1)
  • (modified) flang/lib/Semantics/CMakeLists.txt (+1)
  • (added) flang/lib/Semantics/openmp-dsa.cpp (+29)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+217-84)
  • (modified) flang/test/Semantics/OpenMP/common-block.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/copyprivate03.f90 (+12)
  • (modified) flang/test/Semantics/OpenMP/default-clause.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/do05-positivecase.f90 (+3-3)
  • (modified) flang/test/Semantics/OpenMP/do20.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/forall.f90 (+2-2)
  • (modified) flang/test/Semantics/OpenMP/implicit-dsa.f90 (+24-11)
  • (modified) flang/test/Semantics/OpenMP/reduction08.f90 (+10-10)
  • (modified) flang/test/Semantics/OpenMP/reduction09.f90 (+7-7)
  • (modified) flang/test/Semantics/OpenMP/reduction11.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/scan2.f90 (+2-2)
  • (modified) flang/test/Semantics/OpenMP/symbol01.f90 (+6-6)
  • (modified) flang/test/Semantics/OpenMP/symbol02.f90 (+4-4)
  • (modified) flang/test/Semantics/OpenMP/symbol03.f90 (+4-4)
  • (modified) flang/test/Semantics/OpenMP/symbol04.f90 (+2-2)
  • (modified) flang/test/Semantics/OpenMP/symbol05.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/symbol06.f90 (+1-1)
  • (modified) flang/test/Semantics/OpenMP/symbol07.f90 (+2-2)
  • (modified) flang/test/Semantics/OpenMP/symbol08.f90 (+18-18)
  • (modified) flang/test/Semantics/OpenMP/symbol09.f90 (+2-2)
diff --git a/flang/include/flang/Semantics/openmp-dsa.h b/flang/include/flang/Semantics/openmp-dsa.h
new file mode 100644
index 0000000000000..4b94a679f29ef
--- /dev/null
+++ b/flang/include/flang/Semantics/openmp-dsa.h
@@ -0,0 +1,20 @@
+//===-- include/flang/Semantics/openmp-dsa.h --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef FORTRAN_SEMANTICS_OPENMP_DSA_H_
+#define FORTRAN_SEMANTICS_OPENMP_DSA_H_
+
+#include "flang/Semantics/symbol.h"
+
+namespace Fortran::semantics {
+
+Symbol::Flags GetSymbolDSA(const Symbol &symbol);
+
+} // namespace Fortran::semantics
+
+#endif // FORTRAN_SEMANTICS_OPENMP_DSA_H_
diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
index 4cded64d170cd..59920e08cc926 100644
--- a/flang/include/flang/Semantics/symbol.h
+++ b/flang/include/flang/Semantics/symbol.h
@@ -785,8 +785,8 @@ class Symbol {
       OmpAllocate, OmpDeclarativeAllocateDirective,
       OmpExecutableAllocateDirective, OmpDeclareSimd, OmpDeclareTarget,
       OmpThreadprivate, OmpDeclareReduction, OmpFlushed, OmpCriticalLock,
-      OmpIfSpecified, OmpNone, OmpPreDetermined, OmpImplicit, OmpDependObject,
-      OmpInclusiveScan, OmpExclusiveScan, OmpInScanReduction);
+      OmpIfSpecified, OmpNone, OmpPreDetermined, OmpExplicit, OmpImplicit,
+      OmpDependObject, OmpInclusiveScan, OmpExclusiveScan, OmpInScanReduction);
   using Flags = common::EnumSet<Flag, Flag_enumSize>;
 
   const Scope &owner() const { return *owner_; }
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index c9e91cf3e8042..86d5e0d37bc38 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -58,6 +58,7 @@
 #include "flang/Optimizer/Transforms/Passes.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Runtime/iostat-consts.h"
+#include "flang/Semantics/openmp-dsa.h"
 #include "flang/Semantics/runtime-type-info.h"
 #include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
@@ -1385,7 +1386,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
     if (isUnordered || sym.has<Fortran::semantics::HostAssocDetails>() ||
         sym.has<Fortran::semantics::UseDetails>()) {
       if (!shallowLookupSymbol(sym) &&
-          !sym.test(Fortran::semantics::Symbol::Flag::OmpShared)) {
+          !GetSymbolDSA(sym).test(
+              Fortran::semantics::Symbol::Flag::OmpShared)) {
         // Do concurrent loop variables are not mapped yet since they are local
         // to the Do concurrent scope (same for OpenMP loops).
         mlir::OpBuilder::InsertPoint insPt = builder->saveInsertionPoint();
diff --git a/flang/lib/Semantics/CMakeLists.txt b/flang/lib/Semantics/CMakeLists.txt
index bd8cc47365f06..18c89587843a9 100644
--- a/flang/lib/Semantics/CMakeLists.txt
+++ b/flang/lib/Semantics/CMakeLists.txt
@@ -32,6 +32,7 @@ add_flang_library(FortranSemantics
   dump-expr.cpp
   expression.cpp
   mod-file.cpp
+  openmp-dsa.cpp
   openmp-modifiers.cpp
   pointer-assignment.cpp
   program-tree.cpp
diff --git a/flang/lib/Semantics/openmp-dsa.cpp b/flang/lib/Semantics/openmp-dsa.cpp
new file mode 100644
index 0000000000000..48aa36febe5c5
--- /dev/null
+++ b/flang/lib/Semantics/openmp-dsa.cpp
@@ -0,0 +1,29 @@
+//===-- flang/lib/Semantics/openmp-dsa.cpp ----------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "flang/Semantics/openmp-dsa.h"
+
+namespace Fortran::semantics {
+
+Symbol::Flags GetSymbolDSA(const Symbol &symbol) {
+  Symbol::Flags dsaFlags{Symbol::Flag::OmpPrivate,
+      Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
+      Symbol::Flag::OmpShared, Symbol::Flag::OmpLinear,
+      Symbol::Flag::OmpReduction};
+  Symbol::Flags dsa{symbol.flags() & dsaFlags};
+  if (dsa.any()) {
+    return dsa;
+  }
+  // If no DSA are set use those from the host associated symbol, if any.
+  if (const auto *details{symbol.detailsIf<HostAssocDetails>()}) {
+    return GetSymbolDSA(details->symbol());
+  }
+  return {};
+}
+
+} // namespace Fortran::semantics
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 9fa7bc8964854..e604bca4213c8 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -19,9 +19,11 @@
 #include "flang/Parser/parse-tree.h"
 #include "flang/Parser/tools.h"
 #include "flang/Semantics/expression.h"
+#include "flang/Semantics/openmp-dsa.h"
 #include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
+#include "llvm/Support/Debug.h"
 #include <list>
 #include <map>
 #include <sstream>
@@ -111,10 +113,9 @@ template <typename T> class DirectiveAttributeVisitor {
   const parser::Name *GetLoopIndex(const parser::DoConstruct &);
   const parser::DoConstruct *GetDoConstructIf(
       const parser::ExecutionPartConstruct &);
-  Symbol *DeclareNewPrivateAccessEntity(const Symbol &, Symbol::Flag, Scope &);
-  Symbol *DeclarePrivateAccessEntity(
-      const parser::Name &, Symbol::Flag, Scope &);
-  Symbol *DeclarePrivateAccessEntity(Symbol &, Symbol::Flag, Scope &);
+  Symbol *DeclareNewAccessEntity(const Symbol &, Symbol::Flag, Scope &);
+  Symbol *DeclareAccessEntity(const parser::Name &, Symbol::Flag, Scope &);
+  Symbol *DeclareAccessEntity(Symbol &, Symbol::Flag, Scope &);
   Symbol *DeclareOrMarkOtherAccessEntity(const parser::Name &, Symbol::Flag);
 
   UnorderedSymbolSet dataSharingAttributeObjects_; // on one directive
@@ -749,10 +750,11 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
 
   Symbol::Flags ompFlagsRequireNewSymbol{Symbol::Flag::OmpPrivate,
       Symbol::Flag::OmpLinear, Symbol::Flag::OmpFirstPrivate,
-      Symbol::Flag::OmpLastPrivate, Symbol::Flag::OmpReduction,
-      Symbol::Flag::OmpCriticalLock, Symbol::Flag::OmpCopyIn,
-      Symbol::Flag::OmpUseDevicePtr, Symbol::Flag::OmpUseDeviceAddr,
-      Symbol::Flag::OmpIsDevicePtr, Symbol::Flag::OmpHasDeviceAddr};
+      Symbol::Flag::OmpLastPrivate, Symbol::Flag::OmpShared,
+      Symbol::Flag::OmpReduction, Symbol::Flag::OmpCriticalLock,
+      Symbol::Flag::OmpCopyIn, Symbol::Flag::OmpUseDevicePtr,
+      Symbol::Flag::OmpUseDeviceAddr, Symbol::Flag::OmpIsDevicePtr,
+      Symbol::Flag::OmpHasDeviceAddr};
 
   Symbol::Flags ompFlagsRequireMark{Symbol::Flag::OmpThreadprivate,
       Symbol::Flag::OmpDeclareTarget, Symbol::Flag::OmpExclusiveScan,
@@ -829,8 +831,24 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
   void IssueNonConformanceWarning(
       llvm::omp::Directive D, parser::CharBlock source);
 
-  void CreateImplicitSymbols(
-      const Symbol *symbol, std::optional<Symbol::Flag> setFlag = std::nullopt);
+  void CreateImplicitSymbols(const Symbol *symbol);
+
+  void AddToContextObjectWithExplicitDSA(Symbol &symbol, Symbol::Flag flag) {
+    AddToContextObjectWithDSA(symbol, flag);
+    if (dataSharingAttributeFlags.test(flag)) {
+      symbol.set(Symbol::Flag::OmpExplicit);
+    }
+  }
+
+  // Clear any previous data-sharing attribute flags and set the new ones.
+  // Needed when setting PreDetermined DSAs, that take precedence over
+  // Implicit ones.
+  void SetSymbolDSA(Symbol &symbol, Symbol::Flags flags) {
+    symbol.flags() &= ~(dataSharingAttributeFlags |
+        Symbol::Flags{Symbol::Flag::OmpExplicit, Symbol::Flag::OmpImplicit,
+            Symbol::Flag::OmpPreDetermined});
+    symbol.flags() |= flags;
+  }
 };
 
 template <typename T>
@@ -867,7 +885,7 @@ const parser::DoConstruct *DirectiveAttributeVisitor<T>::GetDoConstructIf(
 }
 
 template <typename T>
-Symbol *DirectiveAttributeVisitor<T>::DeclareNewPrivateAccessEntity(
+Symbol *DirectiveAttributeVisitor<T>::DeclareNewAccessEntity(
     const Symbol &object, Symbol::Flag flag, Scope &scope) {
   assert(object.owner() != currScope());
   auto &symbol{MakeAssocSymbol(object.name(), object, scope)};
@@ -880,20 +898,20 @@ Symbol *DirectiveAttributeVisitor<T>::DeclareNewPrivateAccessEntity(
 }
 
 template <typename T>
-Symbol *DirectiveAttributeVisitor<T>::DeclarePrivateAccessEntity(
+Symbol *DirectiveAttributeVisitor<T>::DeclareAccessEntity(
     const parser::Name &name, Symbol::Flag flag, Scope &scope) {
   if (!name.symbol) {
     return nullptr; // not resolved by Name Resolution step, do nothing
   }
-  name.symbol = DeclarePrivateAccessEntity(*name.symbol, flag, scope);
+  name.symbol = DeclareAccessEntity(*name.symbol, flag, scope);
   return name.symbol;
 }
 
 template <typename T>
-Symbol *DirectiveAttributeVisitor<T>::DeclarePrivateAccessEntity(
+Symbol *DirectiveAttributeVisitor<T>::DeclareAccessEntity(
     Symbol &object, Symbol::Flag flag, Scope &scope) {
   if (object.owner() != currScope()) {
-    return DeclareNewPrivateAccessEntity(object, flag, scope);
+    return DeclareNewAccessEntity(object, flag, scope);
   } else {
     object.set(flag);
     return &object;
@@ -1600,6 +1618,20 @@ void AccAttributeVisitor::CheckMultipleAppearances(
   }
 }
 
+#ifndef NDEBUG
+
+#define DEBUG_TYPE "omp"
+
+static llvm::raw_ostream &operator<<(
+    llvm::raw_ostream &os, const Symbol::Flags &flags);
+
+namespace dbg {
+static void DumpAssocSymbols(llvm::raw_ostream &os, const Symbol &sym);
+static std::string ScopeSourcePos(const Fortran::semantics::Scope &scope);
+} // namespace dbg
+
+#endif
+
 bool OmpAttributeVisitor::Pre(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
@@ -1792,12 +1824,12 @@ void OmpAttributeVisitor::ResolveSeqLoopIndexInParallelOrTaskConstruct(
       }
     }
   }
-  // If this symbol is already Private or Firstprivate in the enclosing
-  // OpenMP parallel or task then there is nothing to do here.
+  // If this symbol already has an explicit data-sharing attribute in the
+  // enclosing OpenMP parallel or task then there is nothing to do here.
   if (auto *symbol{targetIt->scope.FindSymbol(iv.source)}) {
     if (symbol->owner() == targetIt->scope) {
-      if (symbol->test(Symbol::Flag::OmpPrivate) ||
-          symbol->test(Symbol::Flag::OmpFirstPrivate)) {
+      if (symbol->test(Symbol::Flag::OmpExplicit) &&
+          (symbol->flags() & dataSharingAttributeFlags).any()) {
         return;
       }
     }
@@ -1806,7 +1838,8 @@ void OmpAttributeVisitor::ResolveSeqLoopIndexInParallelOrTaskConstruct(
   // parallel or task
   if (auto *symbol{ResolveOmp(iv, Symbol::Flag::OmpPrivate, targetIt->scope)}) {
     targetIt++;
-    symbol->set(Symbol::Flag::OmpPreDetermined);
+    SetSymbolDSA(
+        *symbol, {Symbol::Flag::OmpPreDetermined, Symbol::Flag::OmpPrivate});
     iv.symbol = symbol; // adjust the symbol within region
     for (auto it{dirContext_.rbegin()}; it != targetIt; ++it) {
       AddToContextObjectWithDSA(*symbol, Symbol::Flag::OmpPrivate, *it);
@@ -1918,7 +1951,7 @@ void OmpAttributeVisitor::PrivatizeAssociatedLoopIndexAndCheckLoopLevel(
       const parser::Name *iv{GetLoopIndex(*loop)};
       if (iv) {
         if (auto *symbol{ResolveOmp(*iv, ivDSA, currScope())}) {
-          symbol->set(Symbol::Flag::OmpPreDetermined);
+          SetSymbolDSA(*symbol, {Symbol::Flag::OmpPreDetermined, ivDSA});
           iv->symbol = symbol; // adjust the symbol within region
           AddToContextObjectWithDSA(*symbol, ivDSA);
         }
@@ -2178,42 +2211,48 @@ static bool IsPrivatizable(const Symbol *sym) {
               misc->kind() != MiscDetails::Kind::ConstructName));
 }
 
-void OmpAttributeVisitor::CreateImplicitSymbols(
-    const Symbol *symbol, std::optional<Symbol::Flag> setFlag) {
+void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
   if (!IsPrivatizable(symbol)) {
     return;
   }
 
+  LLVM_DEBUG(llvm::dbgs() << "CreateImplicitSymbols: " << *symbol << '\n');
+
   // Implicitly determined DSAs
   // OMP 5.2 5.1.1 - Variables Referenced in a Construct
   Symbol *lastDeclSymbol = nullptr;
-  std::optional<Symbol::Flag> prevDSA;
+  Symbol::Flags prevDSA;
   for (int dirDepth{0}; dirDepth < (int)dirContext_.size(); ++dirDepth) {
     DirContext &dirContext = dirContext_[dirDepth];
-    std::optional<Symbol::Flag> dsa;
+    Symbol::Flags dsa;
 
-    for (auto symMap : dirContext.objectWithDSA) {
-      // if the `symbol` already has a data-sharing attribute
-      if (symMap.first->name() == symbol->name()) {
-        dsa = symMap.second;
-        break;
+    Scope &scope{context_.FindScope(dirContext.directiveSource)};
+    auto it{scope.find(symbol->name())};
+    if (it != scope.end()) {
+      // There is already a symbol in the current scope, use its DSA.
+      dsa = GetSymbolDSA(*it->second);
+    } else {
+      for (auto symMap : dirContext.objectWithDSA) {
+        if (symMap.first->name() == symbol->name()) {
+          // `symbol` already has a data-sharing attribute in the current
+          // context, use it.
+          dsa.set(symMap.second);
+          break;
+        }
       }
     }
 
     // When handling each implicit rule for a given symbol, one of the
-    // following 3 actions may be taken:
-    // 1. Declare a new private symbol.
-    // 2. Create a new association symbol with no flags, that will represent
-    //    a shared symbol in the current scope. Note that symbols without
-    //    any private flags are considered as shared.
-    // 3. Use the last declared private symbol, by inserting a new symbol
-    //    in the scope being processed, associated with it.
-    //    If no private symbol was declared previously, then no association
-    //    is needed and the symbol from the enclosing scope will be
-    //    inherited by the current one.
+    // following actions may be taken:
+    // 1. Declare a new private or shared symbol.
+    // 2. Use the last declared symbol, by inserting a new symbol in the
+    //    scope being processed, associated with it.
+    //    If no symbol was declared previously, then no association is needed
+    //    and the symbol from the enclosing scope will be inherited by the
+    //    current one.
     //
     // Because of how symbols are collected in lowering, not inserting a new
-    // symbol in the last case could lead to the conclusion that a symbol
+    // symbol in the second case could lead to the conclusion that a symbol
     // from an enclosing construct was declared in the current construct,
     // which would result in wrong privatization code being generated.
     // Consider the following example:
@@ -2231,46 +2270,71 @@ void OmpAttributeVisitor::CreateImplicitSymbols(
     // it would have the private flag set.
     // This would make x appear to be defined in p2, causing it to be
     // privatized in p2 and its privatization in p1 to be skipped.
-    auto makePrivateSymbol = [&](Symbol::Flag flag) {
+    auto makeSymbol = [&](Symbol::Flags flags) {
       const Symbol *hostSymbol =
           lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
-      lastDeclSymbol = DeclareNewPrivateAccessEntity(
+      assert(flags.LeastElement());
+      Symbol::Flag flag = *flags.LeastElement();
+      lastDeclSymbol = DeclareNewAccessEntity(
           *hostSymbol, flag, context_.FindScope(dirContext.directiveSource));
-      if (setFlag) {
-        lastDeclSymbol->set(*setFlag);
-      }
+      lastDeclSymbol->flags() |= flags;
       return lastDeclSymbol;
     };
-    auto makeSharedSymbol = [&](std::optional<Symbol::Flag> flag = {}) {
-      const Symbol *hostSymbol =
-          lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
-      Symbol &assocSymbol = MakeAssocSymbol(symbol->name(), *hostSymbol,
-          context_.FindScope(dirContext.directiveSource));
-      if (flag) {
-        assocSymbol.set(*flag);
-      }
-    };
     auto useLastDeclSymbol = [&]() {
       if (lastDeclSymbol) {
-        makeSharedSymbol();
+        const Symbol *hostSymbol =
+            lastDeclSymbol ? lastDeclSymbol : &symbol->GetUltimate();
+        MakeAssocSymbol(symbol->name(), *hostSymbol,
+            context_.FindScope(dirContext.directiveSource));
       }
     };
 
+#ifndef NDEBUG
+    auto printImplicitRule = [&](const char *id) {
+      LLVM_DEBUG(llvm::dbgs() << "\t" << id << ": dsa: " << dsa << '\n');
+      LLVM_DEBUG(
+          llvm::dbgs() << "\t\tScope: " << dbg::ScopeSourcePos(scope) << '\n');
+    };
+#define PRINT_IMPLICIT_RULE(id) printImplicitRule(id)
+#else
+#define PRINT_IMPLICIT_RULE(id)
+#endif
+
     bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
     bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
     bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
     bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);
 
-    if (dsa.has_value()) {
-      if (dsa.value() == Symbol::Flag::OmpShared &&
-          (parallelDir || taskGenDir || teamsDir)) {
-        makeSharedSymbol(Symbol::Flag::OmpShared);
+    if (dsa.any()) {
+      if (parallelDir || taskGenDir || teamsDir) {
+        Symbol *prevDeclSymbol{lastDeclSymbol};
+        // NOTE As `dsa` will match that of the symbol in the current scope
+        //      (if any), we won't override the DSA of any existing symbol.
+        if ((dsa & dataSharingAttributeFlags).any()) {
+          makeSymbol(dsa);
+        }
+        // Fix host association of explicit symbols, as they can be created
+        // before implicit ones in enclosing scope.
+        if (prevDeclSymbol && prevDeclSymbol != lastDeclSymbol &&
+            lastDeclSymbol->test(Symbol::Flag::OmpExplicit)) {
+          const auto *hostAssoc{lastDeclSymbol->detailsIf<HostAssocDetails>()};
+          if (hostAssoc && hostAssoc->symbol() != *prevDeclSymbol) {
+            lastDeclSymbol->set_details(HostAssocDetails{*prevDeclSymbol});
+          }
+        }
       }
-      // Private symbols will have been declared already.
       prevDSA = dsa;
+      PRINT_IMPLICIT_RULE("0) already has DSA");
       continue;
     }
 
+    // NOTE Because of how lowering uses OmpImplicit flag, we can only set it
+    //      for symbols with private DSA.
+    //      Also, as the default clause is handled separately in lowering,
+    //      don't mark its symbols with OmpImplicit either.
+    //      Ideally, lowering should be changed and all implicit symbols
+    //      should be marked with OmpImplicit.
+
     if (dirContext.defaultDSA == Symbol::Flag::OmpPrivate ||
         dirContext.defaultDSA == Symbol::Flag::OmpFirstPrivate ||
         dirContext.defaultDSA == Symbol::Flag::OmpShared) {
@@ -2279,33 +2343,34 @@ void OmpAttributeVisitor::CreateImplicitSymbols(
       if (!parallelDir && !taskGenDir && !teamsDir) {
         return;
       }
-      if (dirContext.defaultDSA != Symbol::Flag::OmpShared) {
-        makePrivateSymbol(dirContext.defaultDSA);
-      } else {
-        makeSharedSymbol();
-      }
-      dsa = dirContext.defaultDSA;
+      dsa = {dirContext.defaultDSA};
+      makeSymbol(dsa);
+      PRINT_IMPLICIT_RULE("1) default");
     } else if (parallelDir) {
       // 2) parallel -> shared
-      makeSharedSymbol();
-      dsa = Symbol::Flag::OmpShared;
+      dsa = {Symbol::Flag::OmpShared};
+      makeSymbol(dsa);
+      PRINT_IMPLICIT_RULE("2) parallel");
     } else if (!taskGenDir && !targetDir) {
       // 3) enclosing context
-      useLastDeclSymbol();
       dsa = prevDSA;
+      useLastDeclSymbol();
+      PRINT_IMPLICIT_RULE("3) enclosing context");
     } 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) {
+      if (prevDSA.test(Symbol::Flag::OmpShared)) {
         // 6) shared in enclosing context -> shared
-        makeSharedSymbol()...
[truncated]

tblah
tblah approved these changes May 30, 2025
View reviewed changes
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

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

LGTM, the approach seems sensible and it is okay with me. Do you plan to follow up with the lowering changes required to clean this up?

@luporl
Copy link
Contributor Author

luporl commented May 30, 2025

Do you plan to follow up with the lowering changes required to clean this up?

Yes, but it may take a while.

luporl added 2 commits June 3, 2025 09:00
@luporl
[flang][OpenMP] Explicitly set Shared DSA in symbols
5366bba 
Before this change, OmpShared was not always set in shared symbols.
Instead, absence of private flags was interpreted as shared DSA.
The problem was that symbols with no flags, with only a host
association, could also mean "has same DSA as in the enclosing
context". Now shared symbols behave the same as private and can be
treated the same way.

Because of the host association symbols with no flags mentioned
above, it was also incorrect to simply test the flags of a given
symbol to find out if it was private or shared. The function
GetSymbolDSA() was added to fix this. It would be better to avoid
the need of these special symbols, but this would require changes
to how symbols are collected in lowering.

Besides that, some semantic checks need to know if a DSA clause
was used or not. To avoid confusing implicit symbols with DSA
clauses a new flag was added: OmpExplicit. It is now set for all
symbols with explicitly determined data-sharing attributes.

With the changes above, AddToContextObjectWithDSA() and the symbol
to DSA map could probably be removed and the DSA could be obtained
directly from the symbol, but this was not attempted.

Some debug messages were also added, with the "omp" DEBUG_TYPE, to
make it easier to debug the creation of implicit symbols and to
visualize all associations of a given symbol.

Fixes llvm#130533
Fixes llvm#140882
@luporl
Add a comment explaining a new test
cf64aac
@luporl luporl force-pushed the luporl-omp-symbols branch from 6c89ac2 to cf64aac Compare June 3, 2025 12:01
@luporl luporl merged commit aac1f85 into llvm:main Jun 3, 2025
11 checks passed
@luporl luporl deleted the luporl-omp-symbols branch June 3, 2025 13:58
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
@luporl @rorth
[flang][OpenMP] Explicitly set Shared DSA in symbols (llvm#142154)
c780fa5 
Before this change, OmpShared was not always set in shared symbols.
Instead, absence of private flags was interpreted as shared DSA.
The problem was that symbols with no flags, with only a host
association, could also mean "has same DSA as in the enclosing
context". Now shared symbols behave the same as private and can be
treated the same way.

Because of the host association symbols with no flags mentioned
above, it was also incorrect to simply test the flags of a given
symbol to find out if it was private or shared. The function
GetSymbolDSA() was added to fix this. It would be better to avoid
the need of these special symbols, but this would require changes
to how symbols are collected in lowering.

Besides that, some semantic checks need to know if a DSA clause
was used or not. To avoid confusing implicit symbols with DSA
clauses a new flag was added: OmpExplicit. It is now set for all
symbols with explicitly determined data-sharing attributes.

With the changes above, AddToContextObjectWithDSA() and the symbol
to DSA map could probably be removed and the DSA could be obtained
directly from the symbol, but this was not attempted.

Some debug messages were also added, with the "omp" DEBUG_TYPE, to
make it easier to debug the creation of implicit symbols and to
visualize all associations of a given symbol.

Fixes llvm#130533
Fixes llvm#140882
@luporl luporl mentioned this pull request Jun 11, 2025
Closed
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
@luporl @DhruvSrivastavaX
[flang][OpenMP] Explicitly set Shared DSA in symbols (llvm#142154)
4bd8add 
Before this change, OmpShared was not always set in shared symbols.
Instead, absence of private flags was interpreted as shared DSA.
The problem was that symbols with no flags, with only a host
association, could also mean "has same DSA as in the enclosing
context". Now shared symbols behave the same as private and can be
treated the same way.

Because of the host association symbols with no flags mentioned
above, it was also incorrect to simply test the flags of a given
symbol to find out if it was private or shared. The function
GetSymbolDSA() was added to fix this. It would be better to avoid
the need of these special symbols, but this would require changes
to how symbols are collected in lowering.

Besides that, some semantic checks need to know if a DSA clause
was used or not. To avoid confusing implicit symbols with DSA
clauses a new flag was added: OmpExplicit. It is now set for all
symbols with explicitly determined data-sharing attributes.

With the changes above, AddToContextObjectWithDSA() and the symbol
to DSA map could probably be removed and the DSA could be obtained
directly from the symbol, but this was not attempted.

Some debug messages were also added, with the "omp" DEBUG_TYPE, to
make it easier to debug the creation of implicit symbols and to
visualize all associations of a given symbol.

Fixes llvm#130533
Fixes llvm#140882
luporl added a commit to luporl/llvm-project that referenced this pull request Jun 13, 2025
@luporl
[flang][OpenMP] Put taskgroup in a new scope
0b8b37b 
Although taskgroup is a privatizing construct, because of
task_reduction clause, a new scope was not being created for it.
This could cause an extra privatization of variables when
taskgroup was lowered, because its scope would be the same as of
the parent privatizing construct.

This fixes regressions in tests 1052_0201 and 1052_0205, from
Fujitsu testsuite.

This issue didn't happen before because implicit symbols were
being created in a different way before llvm#142154.
@luporl luporl mentioned this pull request Jun 13, 2025
Merged
luporl added a commit that referenced this pull request Jun 16, 2025
@luporl
[flang][OpenMP] Put taskgroup in a new scope (#144122)
1bd4f97 
Although taskgroup is a privatizing construct, because of
task_reduction clause, a new scope was not being created for it.
This could cause an extra privatization of variables when
taskgroup was lowered, because its scope would be the same as of
the parent privatizing construct.

This fixes regressions in tests 1052_0201 and 1052_0205, from
Fujitsu testsuite.

This issue didn't happen before because implicit symbols were
being created in a different way before #142154.
fschlimb pushed a commit to fschlimb/llvm-project that referenced this pull request Jun 18, 2025
@luporl @fschlimb
[flang][OpenMP] Put taskgroup in a new scope (llvm#144122)
c01e6e3 
Although taskgroup is a privatizing construct, because of
task_reduction clause, a new scope was not being created for it.
This could cause an extra privatization of variables when
taskgroup was lowered, because its scope would be the same as of
the parent privatizing construct.

This fixes regressions in tests 1052_0201 and 1052_0205, from
Fujitsu testsuite.

This issue didn't happen before because implicit symbols were
being created in a different way before llvm#142154.
akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
@luporl @akuhlens
[flang][OpenMP] Put taskgroup in a new scope (llvm#144122)
ae7dd75 
Although taskgroup is a privatizing construct, because of
task_reduction clause, a new scope was not being created for it.
This could cause an extra privatization of variables when
taskgroup was lowered, because its scope would be the same as of
the parent privatizing construct.

This fixes regressions in tests 1052_0201 and 1052_0205, from
Fujitsu testsuite.

This issue didn't happen before because implicit symbols were
being created in a different way before llvm#142154.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tblah tblah tblah approved these changes

@Leporacanthicus Leporacanthicus Awaiting requested review from Leporacanthicus

@NimishMishra NimishMishra Awaiting requested review from NimishMishra

@kiranchandramohan kiranchandramohan Awaiting requested review from kiranchandramohan

@raghavendhra raghavendhra Awaiting requested review from raghavendhra

Assignees
No one assigned
Labels
flang:fir-hlfir flang:openmp flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Milestone
No milestone
3 participants
@luporl @llvmbot @tblah