[flang][OpenMP] Diagnose non-variable symbols in OpenMP clauses #111394
+108
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original motivation came from this scenario: ``` !$omp parallel do shared(xyz) xyz: do i = 1, 100 enddo xyz ``` Implement a general check for validity of items listed in OpenMP clauses. In most cases they need to be variables, some clauses allow "extended list items", i.e. variables or procedures.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesThe original motivation came from this scenario: Implement a general check for validity of items listed in OpenMP clauses. In most cases they need to be variables, some clauses allow "extended list items", i.e. variables or procedures. Full diff: https://github.com/llvm/llvm-project/pull/111394.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 5ef504aa72326e..110b52d849d1ed 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -200,6 +200,10 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) {
return CheckAllowed(clause);
}
+bool OmpStructureChecker::IsExtendedListItem(const Symbol &sym) {
+ return evaluate::IsVariable(sym) || sym.IsSubprogram();
+}
+
bool OmpStructureChecker::IsCloselyNestedRegion(const OmpDirectiveSet &set) {
// Definition of close nesting:
//
@@ -454,6 +458,14 @@ void OmpStructureChecker::Enter(const parser::OpenMPConstruct &x) {
}
}
+void OmpStructureChecker::Leave(const parser::OpenMPConstruct &) {
+ for (const auto &[sym, source] : deferredNonVariables_) {
+ context_.SayWithDecl(
+ *sym, source, "'%s' must be a variable"_err_en_US, sym->name());
+ }
+ deferredNonVariables_.clear();
+}
+
void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
const auto &beginDir{std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
@@ -1246,7 +1258,8 @@ void OmpStructureChecker::Leave(const parser::OmpDeclareTargetWithClause &x) {
context_.Say(x.source,
"If the DECLARE TARGET directive has a clause, it must contain at least one ENTER clause or LINK clause"_err_en_US);
}
- if (toClause) {
+ unsigned version{context_.langOptions().OpenMPVersion};
+ if (toClause && version >= 52) {
context_.Warn(common::UsageWarning::OpenMPUsage, toClause->source,
"The usage of TO clause on DECLARE TARGET directive has been deprecated. Use ENTER clause instead."_warn_en_US);
}
@@ -2318,6 +2331,31 @@ void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
void OmpStructureChecker::Enter(const parser::OmpClause &x) {
SetContextClause(x);
+
+ llvm::omp::Clause clauseId = std::visit(
+ [this](auto &&s) { return GetClauseKindForParserClass(s); }, x.u);
+
+ // The visitors for these clauses do their own checks.
+ switch (clauseId) {
+ case llvm::omp::Clause::OMPC_copyprivate:
+ case llvm::omp::Clause::OMPC_enter:
+ case llvm::omp::Clause::OMPC_lastprivate:
+ case llvm::omp::Clause::OMPC_reduction:
+ case llvm::omp::Clause::OMPC_to:
+ return;
+ default:
+ break;
+ }
+
+ if (const parser::OmpObjectList *objList{GetOmpObjectList(x)}) {
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(*objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!evaluate::IsVariable(sym)) {
+ deferredNonVariables_.insert({sym, source});
+ }
+ }
+ }
}
// Following clauses do not have a separate node in parse-tree.h.
@@ -2365,7 +2403,6 @@ CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
CHECK_SIMPLE_CLAUSE(Simd, OMPC_simd)
CHECK_SIMPLE_CLAUSE(Sizes, OMPC_sizes)
CHECK_SIMPLE_CLAUSE(TaskReduction, OMPC_task_reduction)
-CHECK_SIMPLE_CLAUSE(To, OMPC_to)
CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
CHECK_SIMPLE_CLAUSE(Unknown, OMPC_unknown)
CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
@@ -2391,7 +2428,6 @@ CHECK_SIMPLE_CLAUSE(CancellationConstructType, OMPC_cancellation_construct_type)
CHECK_SIMPLE_CLAUSE(Doacross, OMPC_doacross)
CHECK_SIMPLE_CLAUSE(OmpxAttribute, OMPC_ompx_attribute)
CHECK_SIMPLE_CLAUSE(OmpxBare, OMPC_ompx_bare)
-CHECK_SIMPLE_CLAUSE(Enter, OMPC_enter)
CHECK_SIMPLE_CLAUSE(Fail, OMPC_fail)
CHECK_SIMPLE_CLAUSE(Weak, OMPC_weak)
@@ -3357,6 +3393,47 @@ void OmpStructureChecker::Enter(const parser::OmpClause::HasDeviceAddr &x) {
}
}
+void OmpStructureChecker::Enter(const parser::OmpClause::Enter &x) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_enter);
+ const parser::OmpObjectList &objList{x.v};
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!IsExtendedListItem(*sym)) {
+ context_.SayWithDecl(*sym, source,
+ "'%s' must be a variable or a procedure"_err_en_US, sym->name());
+ }
+ }
+}
+
+void OmpStructureChecker::Enter(const parser::OmpClause::To &x) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_to);
+ if (dirContext_.empty()) {
+ return;
+ }
+ // The "to" clause is only allowed on "declare target" (pre-5.1), and
+ // "target update". In the former case it can take an extended list item,
+ // in the latter a variable (a locator).
+
+ // The "declare target" construct (and the "to" clause on it) are already
+ // handled (in the declare-target checkers), so just look at "to" in "target
+ // update".
+ if (GetContext().directive == llvm::omp::OMPD_declare_target) {
+ return;
+ }
+ assert(GetContext().directive == llvm::omp::OMPD_target_update);
+
+ const parser::OmpObjectList &objList{x.v};
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!evaluate::IsVariable(*sym)) {
+ context_.SayWithDecl(
+ *sym, source, "'%s' must be a variable"_err_en_US, sym->name());
+ }
+ }
+}
+
llvm::StringRef OmpStructureChecker::getClauseName(llvm::omp::Clause clause) {
return llvm::omp::getOpenMPClauseName(clause);
}
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 605f3f05b4bc83..e6863b53ecfdef 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -69,6 +69,7 @@ class OmpStructureChecker
using llvmOmpClause = const llvm::omp::Clause;
void Enter(const parser::OpenMPConstruct &);
+ void Leave(const parser::OpenMPConstruct &);
void Enter(const parser::OpenMPLoopConstruct &);
void Leave(const parser::OpenMPLoopConstruct &);
void Enter(const parser::OmpEndLoopDirective &);
@@ -140,6 +141,7 @@ class OmpStructureChecker
private:
bool CheckAllowedClause(llvmOmpClause clause);
+ bool IsExtendedListItem(const Symbol &sym);
void CheckMultipleOccurrence(semantics::UnorderedSymbolSet &listVars,
const std::list<parser::Name> &nameList, const parser::CharBlock &item,
const std::string &clauseName);
@@ -246,6 +248,8 @@ class OmpStructureChecker
LastType
};
int directiveNest_[LastType + 1] = {0};
+
+ SymbolSourceMap deferredNonVariables_;
};
} // namespace Fortran::semantics
#endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_
diff --git a/flang/test/Semantics/OpenMP/name-conflict.f90 b/flang/test/Semantics/OpenMP/name-conflict.f90
new file mode 100644
index 00000000000000..5babc3c6d38866
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/name-conflict.f90
@@ -0,0 +1,24 @@
+!RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+module m
+
+contains
+
+subroutine foo1()
+ integer :: baz1
+!ERROR: 'baz1' must be a variable
+!$omp parallel do shared(baz1)
+ baz1: do i = 1, 100
+ enddo baz1
+!$omp end parallel do
+end subroutine
+
+subroutine foo2()
+ !implicit baz2
+!ERROR: 'baz2' must be a variable
+!$omp parallel do shared(baz2)
+ baz2: do i = 1, 100
+ enddo baz2
+!$omp end parallel do
+end subroutine
+
+end module m
|
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesThe original motivation came from this scenario: Implement a general check for validity of items listed in OpenMP clauses. In most cases they need to be variables, some clauses allow "extended list items", i.e. variables or procedures. Full diff: https://github.com/llvm/llvm-project/pull/111394.diff 3 Files Affected:
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 5ef504aa72326e..110b52d849d1ed 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -200,6 +200,10 @@ bool OmpStructureChecker::CheckAllowedClause(llvmOmpClause clause) {
return CheckAllowed(clause);
}
+bool OmpStructureChecker::IsExtendedListItem(const Symbol &sym) {
+ return evaluate::IsVariable(sym) || sym.IsSubprogram();
+}
+
bool OmpStructureChecker::IsCloselyNestedRegion(const OmpDirectiveSet &set) {
// Definition of close nesting:
//
@@ -454,6 +458,14 @@ void OmpStructureChecker::Enter(const parser::OpenMPConstruct &x) {
}
}
+void OmpStructureChecker::Leave(const parser::OpenMPConstruct &) {
+ for (const auto &[sym, source] : deferredNonVariables_) {
+ context_.SayWithDecl(
+ *sym, source, "'%s' must be a variable"_err_en_US, sym->name());
+ }
+ deferredNonVariables_.clear();
+}
+
void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
const auto &beginDir{std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
@@ -1246,7 +1258,8 @@ void OmpStructureChecker::Leave(const parser::OmpDeclareTargetWithClause &x) {
context_.Say(x.source,
"If the DECLARE TARGET directive has a clause, it must contain at least one ENTER clause or LINK clause"_err_en_US);
}
- if (toClause) {
+ unsigned version{context_.langOptions().OpenMPVersion};
+ if (toClause && version >= 52) {
context_.Warn(common::UsageWarning::OpenMPUsage, toClause->source,
"The usage of TO clause on DECLARE TARGET directive has been deprecated. Use ENTER clause instead."_warn_en_US);
}
@@ -2318,6 +2331,31 @@ void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
void OmpStructureChecker::Enter(const parser::OmpClause &x) {
SetContextClause(x);
+
+ llvm::omp::Clause clauseId = std::visit(
+ [this](auto &&s) { return GetClauseKindForParserClass(s); }, x.u);
+
+ // The visitors for these clauses do their own checks.
+ switch (clauseId) {
+ case llvm::omp::Clause::OMPC_copyprivate:
+ case llvm::omp::Clause::OMPC_enter:
+ case llvm::omp::Clause::OMPC_lastprivate:
+ case llvm::omp::Clause::OMPC_reduction:
+ case llvm::omp::Clause::OMPC_to:
+ return;
+ default:
+ break;
+ }
+
+ if (const parser::OmpObjectList *objList{GetOmpObjectList(x)}) {
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(*objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!evaluate::IsVariable(sym)) {
+ deferredNonVariables_.insert({sym, source});
+ }
+ }
+ }
}
// Following clauses do not have a separate node in parse-tree.h.
@@ -2365,7 +2403,6 @@ CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
CHECK_SIMPLE_CLAUSE(Simd, OMPC_simd)
CHECK_SIMPLE_CLAUSE(Sizes, OMPC_sizes)
CHECK_SIMPLE_CLAUSE(TaskReduction, OMPC_task_reduction)
-CHECK_SIMPLE_CLAUSE(To, OMPC_to)
CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
CHECK_SIMPLE_CLAUSE(Unknown, OMPC_unknown)
CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
@@ -2391,7 +2428,6 @@ CHECK_SIMPLE_CLAUSE(CancellationConstructType, OMPC_cancellation_construct_type)
CHECK_SIMPLE_CLAUSE(Doacross, OMPC_doacross)
CHECK_SIMPLE_CLAUSE(OmpxAttribute, OMPC_ompx_attribute)
CHECK_SIMPLE_CLAUSE(OmpxBare, OMPC_ompx_bare)
-CHECK_SIMPLE_CLAUSE(Enter, OMPC_enter)
CHECK_SIMPLE_CLAUSE(Fail, OMPC_fail)
CHECK_SIMPLE_CLAUSE(Weak, OMPC_weak)
@@ -3357,6 +3393,47 @@ void OmpStructureChecker::Enter(const parser::OmpClause::HasDeviceAddr &x) {
}
}
+void OmpStructureChecker::Enter(const parser::OmpClause::Enter &x) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_enter);
+ const parser::OmpObjectList &objList{x.v};
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!IsExtendedListItem(*sym)) {
+ context_.SayWithDecl(*sym, source,
+ "'%s' must be a variable or a procedure"_err_en_US, sym->name());
+ }
+ }
+}
+
+void OmpStructureChecker::Enter(const parser::OmpClause::To &x) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_to);
+ if (dirContext_.empty()) {
+ return;
+ }
+ // The "to" clause is only allowed on "declare target" (pre-5.1), and
+ // "target update". In the former case it can take an extended list item,
+ // in the latter a variable (a locator).
+
+ // The "declare target" construct (and the "to" clause on it) are already
+ // handled (in the declare-target checkers), so just look at "to" in "target
+ // update".
+ if (GetContext().directive == llvm::omp::OMPD_declare_target) {
+ return;
+ }
+ assert(GetContext().directive == llvm::omp::OMPD_target_update);
+
+ const parser::OmpObjectList &objList{x.v};
+ SymbolSourceMap symbols;
+ GetSymbolsInObjectList(objList, symbols);
+ for (const auto &[sym, source] : symbols) {
+ if (!evaluate::IsVariable(*sym)) {
+ context_.SayWithDecl(
+ *sym, source, "'%s' must be a variable"_err_en_US, sym->name());
+ }
+ }
+}
+
llvm::StringRef OmpStructureChecker::getClauseName(llvm::omp::Clause clause) {
return llvm::omp::getOpenMPClauseName(clause);
}
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 605f3f05b4bc83..e6863b53ecfdef 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -69,6 +69,7 @@ class OmpStructureChecker
using llvmOmpClause = const llvm::omp::Clause;
void Enter(const parser::OpenMPConstruct &);
+ void Leave(const parser::OpenMPConstruct &);
void Enter(const parser::OpenMPLoopConstruct &);
void Leave(const parser::OpenMPLoopConstruct &);
void Enter(const parser::OmpEndLoopDirective &);
@@ -140,6 +141,7 @@ class OmpStructureChecker
private:
bool CheckAllowedClause(llvmOmpClause clause);
+ bool IsExtendedListItem(const Symbol &sym);
void CheckMultipleOccurrence(semantics::UnorderedSymbolSet &listVars,
const std::list<parser::Name> &nameList, const parser::CharBlock &item,
const std::string &clauseName);
@@ -246,6 +248,8 @@ class OmpStructureChecker
LastType
};
int directiveNest_[LastType + 1] = {0};
+
+ SymbolSourceMap deferredNonVariables_;
};
} // namespace Fortran::semantics
#endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_
diff --git a/flang/test/Semantics/OpenMP/name-conflict.f90 b/flang/test/Semantics/OpenMP/name-conflict.f90
new file mode 100644
index 00000000000000..5babc3c6d38866
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/name-conflict.f90
@@ -0,0 +1,24 @@
+!RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+module m
+
+contains
+
+subroutine foo1()
+ integer :: baz1
+!ERROR: 'baz1' must be a variable
+!$omp parallel do shared(baz1)
+ baz1: do i = 1, 100
+ enddo baz1
+!$omp end parallel do
+end subroutine
+
+subroutine foo2()
+ !implicit baz2
+!ERROR: 'baz2' must be a variable
+!$omp parallel do shared(baz2)
+ baz2: do i = 1, 100
+ enddo baz2
+!$omp end parallel do
+end subroutine
+
+end module m
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
raghavendhra
approved these changes
Oct 8, 2024
| end subroutine | ||
|
|
||
| subroutine foo2() | ||
| !implicit baz2 |
There was a problem hiding this comment.
Nit : If you are not planning to use this commented implicit can you please remove it?
There was a problem hiding this comment.
It's there to say that baz2 is implicit...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original motivation came from this scenario:
Implement a general check for validity of items listed in OpenMP clauses. In most cases they need to be variables, some clauses allow "extended list items", i.e. variables or procedures.