[Flang][Semantics] Allow declare target to be used on functions external to the declare targets scope #122546

agozillon · 2025-01-10T23:05:51Z

Whilst a little contrived, OpenMP allows you to utilise declare target in the scope of one function to mark another function declare target, currently this leads to a semantic error.

This appears to be because when we process the declare target directive in the scope of another function (referring to another function), we do not search externally from that functions scope to find possible prior definitions, we only search in the current scope, this leads to us implicitly defining a new variable and using that when implicit none is not specified and then error'ng out or error'ng out earlier when implict none is defined. This patch tries to address this behaviour by looking externally for a function first and using that, before defaulting back to the prior behaviour.

llvmbot · 2025-01-10T23:06:28Z

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: None (agozillon)

Changes

Whilst a little contrived, OpenMP allows you to utilise declare target in the scope of one function to mark another function declare target, currently this leads to a semantic error.

This appears to be because when we process the declare target directive in the scope of another function (referring to another function), we do not search externally from that functions scope to find possible prior definitions, we only search in the current scope, this leads to us implicitly defining a new variable and using that when implicit none is not specified and then error'ng out or error'ng out earlier when implict none is defined. This patch tries to address this behaviour by looking externally for a function first and using that, before defaulting back to the prior behaviour.

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

2 Files Affected:

(modified) flang/lib/Semantics/resolve-names.cpp (+86-2)
(added) flang/test/Semantics/OpenMP/declare-target08.f90 (+41)

diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 51e7c5960dc2ef..44bd540a4228b0 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -736,6 +736,8 @@ class ScopeHandler : public ImplicitRulesVisitor {
     std::vector<const std::list<parser::EquivalenceObject> *> equivalenceSets;
     // Names of all common block objects in the scope
     std::set<SourceName> commonBlockObjects;
+    // Names of all names that show in a declare target declaration
+    std::set<SourceName> declareTargetNames;
     // Info about SAVE statements and attributes in current scope
     struct {
       std::optional<SourceName> saveAll; // "SAVE" without entity list
@@ -1223,6 +1225,7 @@ class DeclarationVisitor : public ArraySpecVisitor,
   const parser::Name *FindComponent(const parser::Name *, const parser::Name &);
   void Initialization(const parser::Name &, const parser::Initialization &,
       bool inComponentDecl);
+  bool FindAndMarkDeclareTargetSymbol(const parser::Name &);
   bool PassesLocalityChecks(
       const parser::Name &name, Symbol &symbol, Symbol::Flag flag);
   bool CheckForHostAssociatedImplicit(const parser::Name &);
@@ -1524,7 +1527,44 @@ class OmpVisitor : public virtual DeclarationVisitor {
     return true;
   }
   void Post(const parser::OpenMPThreadprivate &) { SkipImplicitTyping(false); }
-  bool Pre(const parser::OpenMPDeclareTargetConstruct &) {
+  bool Pre(const parser::OpenMPDeclareTargetConstruct &x) {
+    const auto &spec{std::get<parser::OmpDeclareTargetSpecifier>(x.t)};
+    auto populateDeclareTargetNames =
+        [this](const parser::OmpObjectList &objectList) {
+          for (const auto &ompObject : objectList.v) {
+            common::visit(
+                common::visitors{
+                    [&](const parser::Designator &designator) {
+                      if (const auto *name{
+                              semantics::getDesignatorNameIfDataRef(
+                                  designator)}) {
+                        specPartState_.declareTargetNames.insert(name->source);
+                      }
+                    },
+                    [&](const parser::Name &name) {
+                      specPartState_.declareTargetNames.insert(name.source);
+                    }},
+                ompObject.u);
+          }
+        };
+
+    if (const auto *objectList{parser::Unwrap<parser::OmpObjectList>(spec.u)}) {
+      populateDeclareTargetNames(*objectList);
+    } else if (const auto *clauseList{
+                 parser::Unwrap<parser::OmpClauseList>(spec.u)}) {
+      for (const auto &clause : clauseList->v) {
+        if (const auto *toClause{std::get_if<parser::OmpClause::To>(&clause.u)}) {
+          populateDeclareTargetNames({std::get<parser::OmpObjectList>(toClause->v.t)});
+        } else if (const auto *linkClause{
+                       std::get_if<parser::OmpClause::Link>(&clause.u)}) {
+            populateDeclareTargetNames(linkClause->v);
+        } else if (const auto *enterClause{
+                     std::get_if<parser::OmpClause::Enter>(&clause.u)}) {
+        populateDeclareTargetNames(enterClause->v);
+        }
+      }
+    }
+
     SkipImplicitTyping(true);
     return true;
   }
@@ -8114,7 +8154,12 @@ const parser::Name *DeclarationVisitor::ResolveDataRef(
 // If implicit types are allowed, ensure name is in the symbol table.
 // Otherwise, report an error if it hasn't been declared.
 const parser::Name *DeclarationVisitor::ResolveName(const parser::Name &name) {
-  FindSymbol(name);
+  if (!FindSymbol(name)) {
+    if (FindAndMarkDeclareTargetSymbol(name)) {
+      return &name;
+    }
+  }
+
   if (CheckForHostAssociatedImplicit(name)) {
     NotePossibleBadForwardRef(name);
     return &name;
@@ -8157,6 +8202,7 @@ const parser::Name *DeclarationVisitor::ResolveName(const parser::Name &name) {
         "Implied DO index '%s' uses itself in its own bounds expressions"_err_en_US,
         name.source);
   }
+
   MakeSymbol(InclusiveScope(), name.source, Attrs{});
   auto *symbol{FindSymbol(name)};
   if (!symbol) {
@@ -8164,6 +8210,7 @@ const parser::Name *DeclarationVisitor::ResolveName(const parser::Name &name) {
         "'%s' from host scoping unit is not accessible due to IMPORT"_err_en_US);
     return nullptr;
   }
+
   ConvertToObjectEntity(*symbol);
   ApplyImplicitRules(*symbol);
   NotePossibleBadForwardRef(name);
@@ -8298,6 +8345,43 @@ const parser::Name *DeclarationVisitor::FindComponent(
   return nullptr;
 }
 
+bool DeclarationVisitor::FindAndMarkDeclareTargetSymbol(const parser::Name &name) {
+  if (!specPartState_.declareTargetNames.empty()) {
+    if (specPartState_.declareTargetNames.find(name.source) !=
+        specPartState_.declareTargetNames.end()) {
+      if (!currScope().IsTopLevel()) {
+        // Search preceding scopes until we find a matching symbol or run out
+        // of scopes to search, we skip the current scope as it's already been
+        // designated as implicit here.
+        Symbol *symbol = nullptr;
+        for (auto *scope = &currScope().parent(); ; scope = &scope->parent()) {
+          symbol = scope->FindSymbol(name.source);
+          if (symbol) {
+            if (symbol->test(Symbol::Flag::Subroutine) ||
+                symbol->test(Symbol::Flag::Function)) {
+              const auto pair{currScope().try_emplace(
+                  symbol->name(), Attrs{}, HostAssocDetails{*symbol})};
+              name.symbol = &*pair.first->second;
+              symbol->test(Symbol::Flag::Subroutine)
+                  ? name.symbol->set(Symbol::Flag::Subroutine)
+                  : name.symbol->set(Symbol::Flag::Function);
+              return true;
+            }
+          }
+
+          // This is our loop exit condition, as parent() has an inbuilt assert
+          // if you call it on a top level scope, rather than returning a null
+          // value.
+          if (scope->IsTopLevel()) {
+            return false;
+          }
+        }
+      }
+    }
+  }
+  return false;
+}
+
 void DeclarationVisitor::Initialization(const parser::Name &name,
     const parser::Initialization &init, bool inComponentDecl) {
   // Traversal of the initializer was deferred to here so that the
diff --git a/flang/test/Semantics/OpenMP/declare-target08.f90 b/flang/test/Semantics/OpenMP/declare-target08.f90
new file mode 100644
index 00000000000000..1438d79d373482
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/declare-target08.f90
@@ -0,0 +1,41 @@
+! RUN: %flang_fc1 -fopenmp -fdebug-dump-symbols %s | FileCheck %s
+
+subroutine bar(i, a)
+    !$omp declare target
+    real :: a
+    integer :: i
+    a = a - i
+end subroutine
+
+function baz(a)
+    !$omp declare target
+    real, intent(in) :: a
+    baz = a
+end function baz
+
+program main
+real a
+!CHECK: bar (Subroutine, OmpDeclareTarget): HostAssoc
+!CHECK: baz (Function, OmpDeclareTarget): HostAssoc
+!$omp declare target(bar)
+!$omp declare target(baz)
+
+a = baz(a)
+call bar(2,a)
+call foo(a)
+return
+end
+
+subroutine foo(a)
+real a
+integer i
+!CHECK: bar (Subroutine, OmpDeclareTarget): HostAssoc
+!CHECK: baz (Function, OmpDeclareTarget): HostAssoc
+!$omp declare target(bar)
+!$omp declare target(baz)
+!$omp target
+    a = baz(a)
+    call bar(i,a)
+!$omp end target
+return
+end

agozillon · 2025-01-10T23:06:48Z

Unsure of the best person/persons to review this, so please do feel free to add other/more reviewers :-)

github-actions · 2025-01-10T23:09:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

agozillon · 2025-01-14T15:53:53Z

Small ping for some attention on this PR if at all possible please! Would be greatly appreciated.

mjklemm

I think this looks good.

kparzysz · 2025-01-17T13:25:12Z

flang/lib/Semantics/resolve-names.cpp

-  bool Pre(const parser::OpenMPDeclareTargetConstruct &) {
+  bool Pre(const parser::OpenMPDeclareTargetConstruct &x) {
+    const auto &spec{std::get<parser::OmpDeclareTargetSpecifier>(x.t)};
+    auto populateDeclareTargetNames =


Please use brace initialization in the FE.

kparzysz · 2025-01-17T13:27:23Z

flang/lib/Semantics/resolve-names.cpp

+                    },
+                    [&](const parser::Name &name) {
+                      specPartState_.declareTargetNames.insert(name.source);
+                    }},


Please add a comma after the lambda's closing bracket (i.e. between the two }} above).

kparzysz · 2025-01-17T13:27:47Z

flang/lib/Semantics/resolve-names.cpp

+        if (const auto *toClause{
+                std::get_if<parser::OmpClause::To>(&clause.u)}) {
+          populateDeclareTargetNames(
+              {std::get<parser::OmpObjectList>(toClause->v.t)});


Unnecessary braces.

kparzysz · 2025-01-17T13:32:03Z

flang/lib/Semantics/resolve-names.cpp

+  if (!specPartState_.declareTargetNames.empty()) {
+    if (specPartState_.declareTargetNames.find(name.source) !=
+        specPartState_.declareTargetNames.end()) {


This could be just

if (specPartState_.declareTargetNames.count(name.source)) ...

kparzysz · 2025-01-17T13:34:26Z

flang/lib/Semantics/resolve-names.cpp

+          symbol = scope->FindSymbol(name.source);
+          if (symbol) {


This could be

if (Symbol *symbol{scope->FindSymbol(name.source)}) { ...

and remove the declaration of "symbol" at like 8357.

kparzysz · 2025-01-17T13:38:27Z

flang/lib/Semantics/resolve-names.cpp

+              const auto pair{currScope().try_emplace(
+                  symbol->name(), Attrs{}, HostAssocDetails{*symbol})};


Maybe add an assertion that this insertion actually succeeded.
Also, consider using a structured binding for readability.

kparzysz · 2025-01-17T13:39:58Z

flang/lib/Semantics/resolve-names.cpp

+                  ? name.symbol->set(Symbol::Flag::Subroutine)
+                  : name.symbol->set(Symbol::Flag::Function);
+              return true;
+            }


What if we find the symbol, but it's not a subroutine/function? Should we keep searching?

Thank you very much @kparzysz I'll try to get to this PR and update it next week!

In this case to be prudent I've opted to make it only resolve for functions/subroutines for the moment. I was a little unsure on if we wanted to make it apply to variables as well, and the only context I can think of (my Fortran is still not the best) that it may apply is with a nested BLOCK/BLOCKs with a declare target inside the block . However, if you believe it makes more sense to apply to variables (or any other declare targettable type) as well I'd be more than happy to do so! :-)

The issue is that when you find the symbol, but it's not a function/subroutine, the loop keeps going into the outer scopes. I think it should stop, since we've found something with the same name. Granted, I'm not a Fortran expert, so maybe it's possible that a function and a variable can somehow share the same name without conflict, but this part of the code got my attention.

Yes, I wasn't too sure what the ideal solution would be in those cases personally, was one of the pieces I was looking for some input on! I can try to adjust it to just break the loop if we find something that's not a function/subroutine and let the original behavior continue in those cases, we can then adjust the behavior further if we find a need for it. Do you think that would be a reasonable approach in these cases for the time being?

Yes, let's just add a break here, and I think it's good to go.

…nal to the declare targets scope Whilst a little contrived, OpenMP allows you to utilise declare target in the scope of one function to mark another function declare target, currently this leads to a semantic error. This appears to be because when we process the declare target directive in the scope of another function (referring to another function), we do not search externally from that functions scope to find possible prior definitions, we only search in the current scope, this leads to us implicitly defining a new variable and using that when implicit none is not specified and then error'ng out or error'ng out earlier when implict none is defined. This patch tries to address this behaviour by looking externally for a function first and using that, before defaulting back to the prior behaviour.

agozillon · 2025-01-20T16:21:26Z

should now incorporate all the requested changes! :-) Thank you very much for the review @kparzysz and @mjklemm

kparzysz

LG. Thanks!

klausler · 2025-01-21T16:26:21Z

flang/lib/Semantics/resolve-names.cpp

+        // Search preceding scopes until we find a matching symbol or run out
+        // of scopes to search, we skip the current scope as it's already been
+        // designated as implicit here.
+        Symbol *symbol = nullptr;


This dead declaration has broken several build bots (https://lab.llvm.org/buildbot/#/builders/89/builds/14834).

Please fix or revert.

agozillon requested review from kparzysz and kiranchandramohan January 10, 2025 23:05

llvmbot added flang Flang issues not falling into any other category flang:openmp flang:semantics labels Jan 10, 2025

agozillon requested a review from tblah January 10, 2025 23:06

agozillon force-pushed the declare-target-bug-fix branch from 63a9a93 to aad66df Compare January 10, 2025 23:09

mjklemm approved these changes Jan 14, 2025

View reviewed changes

kparzysz reviewed Jan 17, 2025

View reviewed changes

agozillon force-pushed the declare-target-bug-fix branch from aad66df to 180641e Compare January 20, 2025 16:20

kparzysz approved these changes Jan 21, 2025

View reviewed changes

agozillon merged commit 8f5df88 into llvm:main Jan 21, 2025
8 checks passed

klausler reviewed Jan 21, 2025

View reviewed changes

		const auto pair{currScope().try_emplace(
		symbol->name(), Attrs{}, HostAssocDetails{*symbol})};

[Flang][Semantics] Allow declare target to be used on functions external to the declare targets scope #122546

[Flang][Semantics] Allow declare target to be used on functions external to the declare targets scope #122546

Uh oh!

Conversation

agozillon commented Jan 10, 2025

Uh oh!

llvmbot commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agozillon commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agozillon commented Jan 14, 2025

Uh oh!

mjklemm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agozillon commented Jan 20, 2025

Uh oh!

kparzysz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025 •

edited

Loading