-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[flang][OpenMP] Make OpenMPCriticalConstruct follow block structure #152007
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
base: users/kparzysz/a07-critical-global
Are you sure you want to change the base?
[flang][OpenMP] Make OpenMPCriticalConstruct follow block structure #152007
Conversation
This allows not having the END CRITICAL directive in certain situations. Update semantic checks and symbol resolution.
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-parser Author: Krzysztof Parzyszek (kparzysz) ChangesThis allows not having the END CRITICAL directive in certain situations. Update semantic checks and symbol resolution. Patch is 24.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/152007.diff 16 Files Affected:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8302e40984af0..e72190f019dd1 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4986,9 +4986,9 @@ struct OmpEndCriticalDirective {
CharBlock source;
std::tuple<Verbatim, std::optional<Name>> t;
};
-struct OpenMPCriticalConstruct {
- TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct);
- std::tuple<OmpCriticalDirective, Block, OmpEndCriticalDirective> t;
+
+struct OpenMPCriticalConstruct : public OmpBlockConstruct {
+ INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct, OmpBlockConstruct);
};
// 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
diff --git a/flang/lib/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h
similarity index 100%
rename from flang/lib/Semantics/openmp-utils.h
rename to flang/include/flang/Semantics/openmp-utils.h
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index d1efd8e8d2ca7..f7a7dd8fbe6a0 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -34,6 +34,7 @@
#include "flang/Parser/openmp-utils.h"
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/openmp-directive-sets.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/tools.h"
#include "flang/Support/Flags.h"
#include "flang/Support/OpenMP-utils.h"
@@ -3797,18 +3798,29 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval,
const parser::OpenMPCriticalConstruct &criticalConstruct) {
- const auto &cd = std::get<parser::OmpCriticalDirective>(criticalConstruct.t);
- List<Clause> clauses =
- makeClauses(std::get<parser::OmpClauseList>(cd.t), semaCtx);
+ const parser::OmpDirectiveSpecification &beginSpec =
+ criticalConstruct.BeginDir();
+ List<Clause> clauses = makeClauses(beginSpec.Clauses(), semaCtx);
ConstructQueue queue{buildConstructQueue(
- converter.getFirOpBuilder().getModule(), semaCtx, eval, cd.source,
+ converter.getFirOpBuilder().getModule(), semaCtx, eval, beginSpec.source,
llvm::omp::Directive::OMPD_critical, clauses)};
- const auto &name = std::get<std::optional<parser::Name>>(cd.t);
+ std::optional<parser::Name> critName;
+ const parser::OmpArgumentList &args = beginSpec.Arguments();
+ if (!args.v.empty()) {
+ // All of these things should be guaranteed to exist after semantic checks.
+ auto *object = parser::Unwrap<parser::OmpObject>(args.v.front());
+ assert(object && "Expecting object as argument");
+ auto *designator = semantics::omp::GetDesignatorFromObj(*object);
+ assert(designator && "Expecting desginator in argument");
+ auto *name = semantics::getDesignatorNameIfDataRef(*designator);
+ assert(name && "Expecting dataref in designator");
+ critName = *name;
+ }
mlir::Location currentLocation = converter.getCurrentLocation();
genCriticalOp(converter, symTable, semaCtx, eval, currentLocation, queue,
- queue.begin(), name);
+ queue.begin(), critName);
}
static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 84d1e81bfd9be..ab23e7d70de4f 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1758,17 +1758,8 @@ TYPE_PARSER(sourced(construct<OpenMPDeclareMapperConstruct>(
TYPE_PARSER(construct<OmpReductionCombiner>(Parser<AssignmentStmt>{}) ||
construct<OmpReductionCombiner>(Parser<FunctionReference>{}))
-// 2.13.2 OMP CRITICAL
-TYPE_PARSER(startOmpLine >>
- sourced(construct<OmpEndCriticalDirective>(
- verbatim("END CRITICAL"_tok), maybe(parenthesized(name)))) /
- endOmpLine)
-TYPE_PARSER(sourced(construct<OmpCriticalDirective>(verbatim("CRITICAL"_tok),
- maybe(parenthesized(name)), Parser<OmpClauseList>{})) /
- endOmpLine)
-
-TYPE_PARSER(construct<OpenMPCriticalConstruct>(
- Parser<OmpCriticalDirective>{}, block, Parser<OmpEndCriticalDirective>{}))
+TYPE_PARSER(construct<OpenMPCriticalConstruct>(OmpBlockConstructParser{
+ llvm::omp::Directive::OMPD_critical}))
// 2.11.3 Executable Allocate directive
TYPE_PARSER(
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 46141e2ccab56..4f8d498972807 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2606,9 +2606,7 @@ class UnparseVisitor {
EndOpenMP();
}
void Unparse(const OpenMPCriticalConstruct &x) {
- Walk(std::get<OmpCriticalDirective>(x.t));
- Walk(std::get<Block>(x.t), "");
- Walk(std::get<OmpEndCriticalDirective>(x.t));
+ Unparse(static_cast<const OmpBlockConstruct &>(x));
}
void Unparse(const OmpDeclareTargetWithList &x) {
Put("("), Walk(x.v), Put(")");
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
index a5fdabf0b103c..fcb0f9ad1e25d 100644
--- a/flang/lib/Semantics/check-omp-atomic.cpp
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -11,13 +11,13 @@
//===----------------------------------------------------------------------===//
#include "check-omp-structure.h"
-#include "openmp-utils.h"
#include "flang/Common/indirection.h"
#include "flang/Evaluate/expression.h"
#include "flang/Evaluate/tools.h"
#include "flang/Parser/char-block.h"
#include "flang/Parser/parse-tree.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "flang/Semantics/type.h"
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index 59d57a2ec7cfb..8dad1f5d605e7 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -13,7 +13,6 @@
#include "check-omp-structure.h"
#include "check-directive-structure.h"
-#include "openmp-utils.h"
#include "flang/Common/idioms.h"
#include "flang/Common/visit.h"
@@ -23,6 +22,7 @@
#include "flang/Parser/parse-tree.h"
#include "flang/Parser/tools.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/semantics.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
diff --git a/flang/lib/Semantics/check-omp-metadirective.cpp b/flang/lib/Semantics/check-omp-metadirective.cpp
index 03487da64f1bf..cf5ea9028edc7 100644
--- a/flang/lib/Semantics/check-omp-metadirective.cpp
+++ b/flang/lib/Semantics/check-omp-metadirective.cpp
@@ -12,8 +12,6 @@
#include "check-omp-structure.h"
-#include "openmp-utils.h"
-
#include "flang/Common/idioms.h"
#include "flang/Common/indirection.h"
#include "flang/Common/visit.h"
@@ -21,6 +19,7 @@
#include "flang/Parser/message.h"
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/tools.h"
#include "llvm/Frontend/OpenMP/OMP.h"
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index a9c56c347877f..cbe6b2c68bf05 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -10,7 +10,6 @@
#include "check-directive-structure.h"
#include "definable.h"
-#include "openmp-utils.h"
#include "resolve-names-utils.h"
#include "flang/Common/idioms.h"
@@ -27,6 +26,7 @@
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-directive-sets.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/scope.h"
#include "flang/Semantics/semantics.h"
#include "flang/Semantics/symbol.h"
@@ -537,14 +537,6 @@ template <typename Checker> struct DirectiveSpellingVisitor {
checker_(x.v.source, Directive::OMPD_assume);
return false;
}
- bool Pre(const parser::OmpCriticalDirective &x) {
- checker_(std::get<parser::Verbatim>(x.t).source, Directive::OMPD_critical);
- return false;
- }
- bool Pre(const parser::OmpEndCriticalDirective &x) {
- checker_(std::get<parser::Verbatim>(x.t).source, Directive::OMPD_critical);
- return false;
- }
bool Pre(const parser::OmpMetadirectiveDirective &x) {
checker_(
std::get<parser::Verbatim>(x.t).source, Directive::OMPD_metadirective);
@@ -2034,41 +2026,87 @@ void OmpStructureChecker::Leave(const parser::OpenMPCancelConstruct &) {
}
void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
- const auto &dir{std::get<parser::OmpCriticalDirective>(x.t)};
- const auto &dirSource{std::get<parser::Verbatim>(dir.t).source};
- const auto &endDir{std::get<parser::OmpEndCriticalDirective>(x.t)};
- PushContextAndClauseSets(dirSource, llvm::omp::Directive::OMPD_critical);
+ const parser::OmpBeginDirective &beginSpec{x.BeginDir()};
+ const std::optional<parser::OmpEndDirective> &endSpec{x.EndDir()};
+ PushContextAndClauseSets(beginSpec.DirName().source, beginSpec.DirName().v);
+
const auto &block{std::get<parser::Block>(x.t)};
- CheckNoBranching(block, llvm::omp::Directive::OMPD_critical, dir.source);
- const auto &dirName{std::get<std::optional<parser::Name>>(dir.t)};
- const auto &endDirName{std::get<std::optional<parser::Name>>(endDir.t)};
- const auto &ompClause{std::get<parser::OmpClauseList>(dir.t)};
- if (dirName && endDirName &&
- dirName->ToString().compare(endDirName->ToString())) {
- context_
- .Say(endDirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(dirName->source, "should be "_en_US);
- } else if (dirName && !endDirName) {
- context_
- .Say(dirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(dirName->source, "should be NULL"_en_US);
- } else if (!dirName && endDirName) {
- context_
- .Say(endDirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(endDirName->source, "should be NULL"_en_US);
- }
- if (!dirName && !ompClause.source.empty() &&
- ompClause.source.NULTerminatedToString() != "hint(omp_sync_hint_none)") {
- context_.Say(dir.source,
- parser::MessageFormattedText{
- "Hint clause other than omp_sync_hint_none cannot be specified for "
- "an unnamed CRITICAL directive"_err_en_US});
+ CheckNoBranching(
+ block, llvm::omp::Directive::OMPD_critical, beginSpec.DirName().source);
+
+ auto getNameFromArg{[](const parser::OmpArgument &arg) {
+ if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
+ if (auto *designator{omp::GetDesignatorFromObj(*object)}) {
+ return getDesignatorNameIfDataRef(*designator);
+ }
+ }
+ return static_cast<const parser::Name *>(nullptr);
+ }};
+
+ auto checkArgumentList{[&](const parser::OmpArgumentList &args) {
+ if (args.v.size() > 1) {
+ context_.Say(args.source,
+ "Only a single argument is allowed in CRITICAL directive"_err_en_US);
+ } else if (!args.v.empty()) {
+ if (!getNameFromArg(args.v.front())) {
+ context_.Say(args.v.front().source,
+ "CRITICAL argument should be a name"_err_en_US);
+ }
+ }
+ }};
+
+ const parser::Name *beginName{nullptr};
+ const parser::Name *endName{nullptr};
+
+ auto &beginArgs{beginSpec.Arguments()};
+ checkArgumentList(beginArgs);
+
+ if (!beginArgs.v.empty()) {
+ beginName = getNameFromArg(beginArgs.v.front());
+ }
+
+ if (endSpec) {
+ auto &endArgs{endSpec->Arguments()};
+ checkArgumentList(endArgs);
+
+ if (beginArgs.v.empty() != endArgs.v.empty()) {
+ parser::CharBlock source{
+ beginArgs.v.empty() ? endArgs.source : beginArgs.source};
+ context_.Say(source,
+ "Either both CRITICAL and END CRITICAL should have an argument, or none of them should"_err_en_US);
+ } else if (!beginArgs.v.empty()) {
+ endName = getNameFromArg(endArgs.v.front());
+ if (beginName && endName) {
+ if (beginName->ToString() != endName->ToString()) {
+ context_.Say(endName->source,
+ "The names on CRITICAL and END CRITICAL must match"_err_en_US);
+ }
+ }
+ }
+ }
+
+ for (auto &clause : beginSpec.Clauses().v) {
+ auto *hint{std::get_if<parser::OmpClause::Hint>(&clause.u)};
+ if (!hint) {
+ continue;
+ }
+ const int64_t OmpSyncHintNone = 0; // omp_sync_hint_none
+ std::optional<int64_t> hintValue{GetIntValue(hint->v.v)};
+ if (hintValue && *hintValue != OmpSyncHintNone) {
+ // Emit a diagnostic if the name is missing, and point to the directive
+ // with a missing name.
+ parser::CharBlock source;
+ if (!beginName) {
+ source = beginSpec.DirName().source;
+ } else if (endSpec && !endName) {
+ source = endSpec->DirName().source;
+ }
+
+ if (!source.empty()) {
+ context_.Say(source,
+ "When HINT other than 'omp_sync_hint_none' is present, CRITICAL directive should have a name"_err_en_US);
+ }
+ }
}
}
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index 7a492a4378907..e8df346ccdc3e 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//
-#include "openmp-utils.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Common/indirection.h"
#include "flang/Common/reference.h"
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 64bb27962faab..7110f607508e7 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -10,7 +10,6 @@
#include "check-acc-structure.h"
#include "check-omp-structure.h"
-#include "openmp-utils.h"
#include "resolve-names-utils.h"
#include "flang/Common/idioms.h"
#include "flang/Evaluate/fold.h"
@@ -22,6 +21,7 @@
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-dsa.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "llvm/Frontend/OpenMP/OMP.h.inc"
@@ -2124,8 +2124,8 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPSectionConstruct &x) {
}
bool OmpAttributeVisitor::Pre(const parser::OpenMPCriticalConstruct &x) {
- const auto &beginCriticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
- PushContext(beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+ const parser::OmpBeginDirective &beginSpec{x.BeginDir()};
+ PushContext(beginSpec.DirName().source, beginSpec.DirName().v);
GetContext().withinConstruct = true;
return true;
}
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 86201ebee8bdf..f066025354253 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -30,6 +30,7 @@
#include "flang/Semantics/attr.h"
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/program-tree.h"
#include "flang/Semantics/scope.h"
#include "flang/Semantics/semantics.h"
@@ -1486,6 +1487,16 @@ class OmpVisitor : public virtual DeclarationVisitor {
void Post(const parser::OpenMPBlockConstruct &);
bool Pre(const parser::OmpBeginDirective &x) {
AddOmpSourceRange(x.source);
+ // Manually resolve names in CRITICAL directives. This is because these
+ // names do not denote Fortran objects, and the CRITICAL directive causes
+ // them to be "auto-declared", i.e. inserted into the global scope.
+ // More specifically, they are not expected to have explicit declarations,
+ // and if they do the behavior is unspeficied.
+ if (x.DirName().v == llvm::omp::Directive::OMPD_critical) {
+ for (const parser::OmpArgument &arg : x.Arguments().v) {
+ ResolveCriticalName(arg);
+ }
+ }
return true;
}
void Post(const parser::OmpBeginDirective &) {
@@ -1493,6 +1504,12 @@ class OmpVisitor : public virtual DeclarationVisitor {
}
bool Pre(const parser::OmpEndDirective &x) {
AddOmpSourceRange(x.source);
+ // Manually resolve names in CRITICAL directives.
+ if (x.DirName().v == llvm::omp::Directive::OMPD_critical) {
+ for (const parser::OmpArgument &arg : x.Arguments().v) {
+ ResolveCriticalName(arg);
+ }
+ }
return true;
}
void Post(const parser::OmpEndDirective &) {
@@ -1591,32 +1608,6 @@ class OmpVisitor : public virtual DeclarationVisitor {
void Post(const parser::OmpEndSectionsDirective &) {
messageHandler().set_currStmtSource(std::nullopt);
}
- bool Pre(const parser::OmpCriticalDirective &x) {
- AddOmpSourceRange(x.source);
- // Manually resolve names in CRITICAL directives. This is because these
- // names do not denote Fortran objects, and the CRITICAL directive causes
- // them to be "auto-declared", i.e. inserted into the global scope.
- // More specifically, they are not expected to have explicit declarations,
- // and if they do the behavior is unspeficied.
- if (auto &maybeName{std::get<std::optional<parser::Name>>(x.t)}) {
- ResolveCriticalName(*maybeName);
- }
- return true;
- }
- void Post(const parser::OmpCriticalDirective &) {
- messageHandler().set_currStmtSource(std::nullopt);
- }
- bool Pre(const parser::OmpEndCriticalDirective &x) {
- AddOmpSourceRange(x.source);
- // Manually resolve names in CRITICAL directives.
- if (auto &maybeName{std::get<std::optional<parser::Name>>(x.t)}) {
- ResolveCriticalName(*maybeName);
- }
- return true;
- }
- void Post(const parser::OmpEndCriticalDirective &) {
- messageHandler().set_currStmtSource(std::nullopt);
- }
bool Pre(const parser::OpenMPThreadprivate &) {
SkipImplicitTyping(true);
return true;
@@ -1732,7 +1723,7 @@ class OmpVisitor : public virtual DeclarationVisitor {
const std::optional<parser::OmpClauseList> &clauses,
const T &wholeConstruct);
- void ResolveCriticalName(const parser::Name &name);
+ void ResolveCriticalName(const parser::OmpArgument &arg);
int metaLevel_{0};
const parser::OmpMetadirectiveDirective *metaDirective_{nullptr};
@@ -1961,7 +1952,7 @@ void OmpVisitor::ProcessReductionSpecifier(
}
}
-void OmpVisitor::ResolveCriticalName(const parser::Name &name) {
+void OmpVisitor::ResolveCriticalName(const parser::OmpArgument &arg) {
auto &globalScope{[&]() -> Scope & {
for (Scope *s{&currScope()};; s = &s->parent()) {
if (s->IsTopLevel()) {
@@ -1979,15 +1970,21 @@ void OmpVisitor::ResolveCriticalName(const parser::Name &name) {
}
}};
- if (auto *symbol{findSymbol(name)}) {
- if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
- SayWithDecl(name, *symbol,
- "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
- name.ToString());
+ if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
+ if (auto *desg{omp::GetDesignatorFromObj(*object)}) {
+ if (auto *name{getDesignatorNameIfDataRef(*desg)}) {
+ if (auto *symbol{findSymbol(*name)}) {
+ if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
+ SayWithDecl(*name, *symbol,
+ "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
+ name->ToString());
+ }
+ } else {
+ name->symbol = &MakeSymbol(globalScope, name->source, Attrs{});
+ name->symbol->set(Symbol::Flag::OmpCriticalLock);
+ }
+ }
}
- } else {
- name.symbol = &MakeSymbol(globalScope, name.source, Attrs{});
- name.symbol->set(Symbol::Flag::OmpCriticalLock);
}
}
diff --git a/flang/lib/Semantics/unparse-with-symbols.cpp b/flang/lib/Semantics/unparse-with-symbols.cpp
index 3093e39ba2411..41077e0e0aad7 100644
--- a...
[truncated]
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesThis allows not having the END CRITICAL directive in certain situations. Update semantic checks and symbol resolution. Patch is 24.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/152007.diff 16 Files Affected:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8302e40984af0..e72190f019dd1 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4986,9 +4986,9 @@ struct OmpEndCriticalDirective {
CharBlock source;
std::tuple<Verbatim, std::optional<Name>> t;
};
-struct OpenMPCriticalConstruct {
- TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct);
- std::tuple<OmpCriticalDirective, Block, OmpEndCriticalDirective> t;
+
+struct OpenMPCriticalConstruct : public OmpBlockConstruct {
+ INHERITED_TUPLE_CLASS_BOILERPLATE(OpenMPCriticalConstruct, OmpBlockConstruct);
};
// 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
diff --git a/flang/lib/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h
similarity index 100%
rename from flang/lib/Semantics/openmp-utils.h
rename to flang/include/flang/Semantics/openmp-utils.h
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index d1efd8e8d2ca7..f7a7dd8fbe6a0 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -34,6 +34,7 @@
#include "flang/Parser/openmp-utils.h"
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/openmp-directive-sets.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/tools.h"
#include "flang/Support/Flags.h"
#include "flang/Support/OpenMP-utils.h"
@@ -3797,18 +3798,29 @@ static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval,
const parser::OpenMPCriticalConstruct &criticalConstruct) {
- const auto &cd = std::get<parser::OmpCriticalDirective>(criticalConstruct.t);
- List<Clause> clauses =
- makeClauses(std::get<parser::OmpClauseList>(cd.t), semaCtx);
+ const parser::OmpDirectiveSpecification &beginSpec =
+ criticalConstruct.BeginDir();
+ List<Clause> clauses = makeClauses(beginSpec.Clauses(), semaCtx);
ConstructQueue queue{buildConstructQueue(
- converter.getFirOpBuilder().getModule(), semaCtx, eval, cd.source,
+ converter.getFirOpBuilder().getModule(), semaCtx, eval, beginSpec.source,
llvm::omp::Directive::OMPD_critical, clauses)};
- const auto &name = std::get<std::optional<parser::Name>>(cd.t);
+ std::optional<parser::Name> critName;
+ const parser::OmpArgumentList &args = beginSpec.Arguments();
+ if (!args.v.empty()) {
+ // All of these things should be guaranteed to exist after semantic checks.
+ auto *object = parser::Unwrap<parser::OmpObject>(args.v.front());
+ assert(object && "Expecting object as argument");
+ auto *designator = semantics::omp::GetDesignatorFromObj(*object);
+ assert(designator && "Expecting desginator in argument");
+ auto *name = semantics::getDesignatorNameIfDataRef(*designator);
+ assert(name && "Expecting dataref in designator");
+ critName = *name;
+ }
mlir::Location currentLocation = converter.getCurrentLocation();
genCriticalOp(converter, symTable, semaCtx, eval, currentLocation, queue,
- queue.begin(), name);
+ queue.begin(), critName);
}
static void genOMP(lower::AbstractConverter &converter, lower::SymMap &symTable,
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 84d1e81bfd9be..ab23e7d70de4f 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1758,17 +1758,8 @@ TYPE_PARSER(sourced(construct<OpenMPDeclareMapperConstruct>(
TYPE_PARSER(construct<OmpReductionCombiner>(Parser<AssignmentStmt>{}) ||
construct<OmpReductionCombiner>(Parser<FunctionReference>{}))
-// 2.13.2 OMP CRITICAL
-TYPE_PARSER(startOmpLine >>
- sourced(construct<OmpEndCriticalDirective>(
- verbatim("END CRITICAL"_tok), maybe(parenthesized(name)))) /
- endOmpLine)
-TYPE_PARSER(sourced(construct<OmpCriticalDirective>(verbatim("CRITICAL"_tok),
- maybe(parenthesized(name)), Parser<OmpClauseList>{})) /
- endOmpLine)
-
-TYPE_PARSER(construct<OpenMPCriticalConstruct>(
- Parser<OmpCriticalDirective>{}, block, Parser<OmpEndCriticalDirective>{}))
+TYPE_PARSER(construct<OpenMPCriticalConstruct>(OmpBlockConstructParser{
+ llvm::omp::Directive::OMPD_critical}))
// 2.11.3 Executable Allocate directive
TYPE_PARSER(
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 46141e2ccab56..4f8d498972807 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2606,9 +2606,7 @@ class UnparseVisitor {
EndOpenMP();
}
void Unparse(const OpenMPCriticalConstruct &x) {
- Walk(std::get<OmpCriticalDirective>(x.t));
- Walk(std::get<Block>(x.t), "");
- Walk(std::get<OmpEndCriticalDirective>(x.t));
+ Unparse(static_cast<const OmpBlockConstruct &>(x));
}
void Unparse(const OmpDeclareTargetWithList &x) {
Put("("), Walk(x.v), Put(")");
diff --git a/flang/lib/Semantics/check-omp-atomic.cpp b/flang/lib/Semantics/check-omp-atomic.cpp
index a5fdabf0b103c..fcb0f9ad1e25d 100644
--- a/flang/lib/Semantics/check-omp-atomic.cpp
+++ b/flang/lib/Semantics/check-omp-atomic.cpp
@@ -11,13 +11,13 @@
//===----------------------------------------------------------------------===//
#include "check-omp-structure.h"
-#include "openmp-utils.h"
#include "flang/Common/indirection.h"
#include "flang/Evaluate/expression.h"
#include "flang/Evaluate/tools.h"
#include "flang/Parser/char-block.h"
#include "flang/Parser/parse-tree.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "flang/Semantics/type.h"
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index 59d57a2ec7cfb..8dad1f5d605e7 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -13,7 +13,6 @@
#include "check-omp-structure.h"
#include "check-directive-structure.h"
-#include "openmp-utils.h"
#include "flang/Common/idioms.h"
#include "flang/Common/visit.h"
@@ -23,6 +22,7 @@
#include "flang/Parser/parse-tree.h"
#include "flang/Parser/tools.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/semantics.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
diff --git a/flang/lib/Semantics/check-omp-metadirective.cpp b/flang/lib/Semantics/check-omp-metadirective.cpp
index 03487da64f1bf..cf5ea9028edc7 100644
--- a/flang/lib/Semantics/check-omp-metadirective.cpp
+++ b/flang/lib/Semantics/check-omp-metadirective.cpp
@@ -12,8 +12,6 @@
#include "check-omp-structure.h"
-#include "openmp-utils.h"
-
#include "flang/Common/idioms.h"
#include "flang/Common/indirection.h"
#include "flang/Common/visit.h"
@@ -21,6 +19,7 @@
#include "flang/Parser/message.h"
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/tools.h"
#include "llvm/Frontend/OpenMP/OMP.h"
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index a9c56c347877f..cbe6b2c68bf05 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -10,7 +10,6 @@
#include "check-directive-structure.h"
#include "definable.h"
-#include "openmp-utils.h"
#include "resolve-names-utils.h"
#include "flang/Common/idioms.h"
@@ -27,6 +26,7 @@
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-directive-sets.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/scope.h"
#include "flang/Semantics/semantics.h"
#include "flang/Semantics/symbol.h"
@@ -537,14 +537,6 @@ template <typename Checker> struct DirectiveSpellingVisitor {
checker_(x.v.source, Directive::OMPD_assume);
return false;
}
- bool Pre(const parser::OmpCriticalDirective &x) {
- checker_(std::get<parser::Verbatim>(x.t).source, Directive::OMPD_critical);
- return false;
- }
- bool Pre(const parser::OmpEndCriticalDirective &x) {
- checker_(std::get<parser::Verbatim>(x.t).source, Directive::OMPD_critical);
- return false;
- }
bool Pre(const parser::OmpMetadirectiveDirective &x) {
checker_(
std::get<parser::Verbatim>(x.t).source, Directive::OMPD_metadirective);
@@ -2034,41 +2026,87 @@ void OmpStructureChecker::Leave(const parser::OpenMPCancelConstruct &) {
}
void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
- const auto &dir{std::get<parser::OmpCriticalDirective>(x.t)};
- const auto &dirSource{std::get<parser::Verbatim>(dir.t).source};
- const auto &endDir{std::get<parser::OmpEndCriticalDirective>(x.t)};
- PushContextAndClauseSets(dirSource, llvm::omp::Directive::OMPD_critical);
+ const parser::OmpBeginDirective &beginSpec{x.BeginDir()};
+ const std::optional<parser::OmpEndDirective> &endSpec{x.EndDir()};
+ PushContextAndClauseSets(beginSpec.DirName().source, beginSpec.DirName().v);
+
const auto &block{std::get<parser::Block>(x.t)};
- CheckNoBranching(block, llvm::omp::Directive::OMPD_critical, dir.source);
- const auto &dirName{std::get<std::optional<parser::Name>>(dir.t)};
- const auto &endDirName{std::get<std::optional<parser::Name>>(endDir.t)};
- const auto &ompClause{std::get<parser::OmpClauseList>(dir.t)};
- if (dirName && endDirName &&
- dirName->ToString().compare(endDirName->ToString())) {
- context_
- .Say(endDirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(dirName->source, "should be "_en_US);
- } else if (dirName && !endDirName) {
- context_
- .Say(dirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(dirName->source, "should be NULL"_en_US);
- } else if (!dirName && endDirName) {
- context_
- .Say(endDirName->source,
- parser::MessageFormattedText{
- "CRITICAL directive names do not match"_err_en_US})
- .Attach(endDirName->source, "should be NULL"_en_US);
- }
- if (!dirName && !ompClause.source.empty() &&
- ompClause.source.NULTerminatedToString() != "hint(omp_sync_hint_none)") {
- context_.Say(dir.source,
- parser::MessageFormattedText{
- "Hint clause other than omp_sync_hint_none cannot be specified for "
- "an unnamed CRITICAL directive"_err_en_US});
+ CheckNoBranching(
+ block, llvm::omp::Directive::OMPD_critical, beginSpec.DirName().source);
+
+ auto getNameFromArg{[](const parser::OmpArgument &arg) {
+ if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
+ if (auto *designator{omp::GetDesignatorFromObj(*object)}) {
+ return getDesignatorNameIfDataRef(*designator);
+ }
+ }
+ return static_cast<const parser::Name *>(nullptr);
+ }};
+
+ auto checkArgumentList{[&](const parser::OmpArgumentList &args) {
+ if (args.v.size() > 1) {
+ context_.Say(args.source,
+ "Only a single argument is allowed in CRITICAL directive"_err_en_US);
+ } else if (!args.v.empty()) {
+ if (!getNameFromArg(args.v.front())) {
+ context_.Say(args.v.front().source,
+ "CRITICAL argument should be a name"_err_en_US);
+ }
+ }
+ }};
+
+ const parser::Name *beginName{nullptr};
+ const parser::Name *endName{nullptr};
+
+ auto &beginArgs{beginSpec.Arguments()};
+ checkArgumentList(beginArgs);
+
+ if (!beginArgs.v.empty()) {
+ beginName = getNameFromArg(beginArgs.v.front());
+ }
+
+ if (endSpec) {
+ auto &endArgs{endSpec->Arguments()};
+ checkArgumentList(endArgs);
+
+ if (beginArgs.v.empty() != endArgs.v.empty()) {
+ parser::CharBlock source{
+ beginArgs.v.empty() ? endArgs.source : beginArgs.source};
+ context_.Say(source,
+ "Either both CRITICAL and END CRITICAL should have an argument, or none of them should"_err_en_US);
+ } else if (!beginArgs.v.empty()) {
+ endName = getNameFromArg(endArgs.v.front());
+ if (beginName && endName) {
+ if (beginName->ToString() != endName->ToString()) {
+ context_.Say(endName->source,
+ "The names on CRITICAL and END CRITICAL must match"_err_en_US);
+ }
+ }
+ }
+ }
+
+ for (auto &clause : beginSpec.Clauses().v) {
+ auto *hint{std::get_if<parser::OmpClause::Hint>(&clause.u)};
+ if (!hint) {
+ continue;
+ }
+ const int64_t OmpSyncHintNone = 0; // omp_sync_hint_none
+ std::optional<int64_t> hintValue{GetIntValue(hint->v.v)};
+ if (hintValue && *hintValue != OmpSyncHintNone) {
+ // Emit a diagnostic if the name is missing, and point to the directive
+ // with a missing name.
+ parser::CharBlock source;
+ if (!beginName) {
+ source = beginSpec.DirName().source;
+ } else if (endSpec && !endName) {
+ source = endSpec->DirName().source;
+ }
+
+ if (!source.empty()) {
+ context_.Say(source,
+ "When HINT other than 'omp_sync_hint_none' is present, CRITICAL directive should have a name"_err_en_US);
+ }
+ }
}
}
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index 7a492a4378907..e8df346ccdc3e 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//
-#include "openmp-utils.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Common/indirection.h"
#include "flang/Common/reference.h"
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 64bb27962faab..7110f607508e7 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -10,7 +10,6 @@
#include "check-acc-structure.h"
#include "check-omp-structure.h"
-#include "openmp-utils.h"
#include "resolve-names-utils.h"
#include "flang/Common/idioms.h"
#include "flang/Evaluate/fold.h"
@@ -22,6 +21,7 @@
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-dsa.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/symbol.h"
#include "flang/Semantics/tools.h"
#include "llvm/Frontend/OpenMP/OMP.h.inc"
@@ -2124,8 +2124,8 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPSectionConstruct &x) {
}
bool OmpAttributeVisitor::Pre(const parser::OpenMPCriticalConstruct &x) {
- const auto &beginCriticalDir{std::get<parser::OmpCriticalDirective>(x.t)};
- PushContext(beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+ const parser::OmpBeginDirective &beginSpec{x.BeginDir()};
+ PushContext(beginSpec.DirName().source, beginSpec.DirName().v);
GetContext().withinConstruct = true;
return true;
}
diff --git a/flang/lib/Semantics/resolve-names.cpp b/flang/lib/Semantics/resolve-names.cpp
index 86201ebee8bdf..f066025354253 100644
--- a/flang/lib/Semantics/resolve-names.cpp
+++ b/flang/lib/Semantics/resolve-names.cpp
@@ -30,6 +30,7 @@
#include "flang/Semantics/attr.h"
#include "flang/Semantics/expression.h"
#include "flang/Semantics/openmp-modifiers.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/program-tree.h"
#include "flang/Semantics/scope.h"
#include "flang/Semantics/semantics.h"
@@ -1486,6 +1487,16 @@ class OmpVisitor : public virtual DeclarationVisitor {
void Post(const parser::OpenMPBlockConstruct &);
bool Pre(const parser::OmpBeginDirective &x) {
AddOmpSourceRange(x.source);
+ // Manually resolve names in CRITICAL directives. This is because these
+ // names do not denote Fortran objects, and the CRITICAL directive causes
+ // them to be "auto-declared", i.e. inserted into the global scope.
+ // More specifically, they are not expected to have explicit declarations,
+ // and if they do the behavior is unspeficied.
+ if (x.DirName().v == llvm::omp::Directive::OMPD_critical) {
+ for (const parser::OmpArgument &arg : x.Arguments().v) {
+ ResolveCriticalName(arg);
+ }
+ }
return true;
}
void Post(const parser::OmpBeginDirective &) {
@@ -1493,6 +1504,12 @@ class OmpVisitor : public virtual DeclarationVisitor {
}
bool Pre(const parser::OmpEndDirective &x) {
AddOmpSourceRange(x.source);
+ // Manually resolve names in CRITICAL directives.
+ if (x.DirName().v == llvm::omp::Directive::OMPD_critical) {
+ for (const parser::OmpArgument &arg : x.Arguments().v) {
+ ResolveCriticalName(arg);
+ }
+ }
return true;
}
void Post(const parser::OmpEndDirective &) {
@@ -1591,32 +1608,6 @@ class OmpVisitor : public virtual DeclarationVisitor {
void Post(const parser::OmpEndSectionsDirective &) {
messageHandler().set_currStmtSource(std::nullopt);
}
- bool Pre(const parser::OmpCriticalDirective &x) {
- AddOmpSourceRange(x.source);
- // Manually resolve names in CRITICAL directives. This is because these
- // names do not denote Fortran objects, and the CRITICAL directive causes
- // them to be "auto-declared", i.e. inserted into the global scope.
- // More specifically, they are not expected to have explicit declarations,
- // and if they do the behavior is unspeficied.
- if (auto &maybeName{std::get<std::optional<parser::Name>>(x.t)}) {
- ResolveCriticalName(*maybeName);
- }
- return true;
- }
- void Post(const parser::OmpCriticalDirective &) {
- messageHandler().set_currStmtSource(std::nullopt);
- }
- bool Pre(const parser::OmpEndCriticalDirective &x) {
- AddOmpSourceRange(x.source);
- // Manually resolve names in CRITICAL directives.
- if (auto &maybeName{std::get<std::optional<parser::Name>>(x.t)}) {
- ResolveCriticalName(*maybeName);
- }
- return true;
- }
- void Post(const parser::OmpEndCriticalDirective &) {
- messageHandler().set_currStmtSource(std::nullopt);
- }
bool Pre(const parser::OpenMPThreadprivate &) {
SkipImplicitTyping(true);
return true;
@@ -1732,7 +1723,7 @@ class OmpVisitor : public virtual DeclarationVisitor {
const std::optional<parser::OmpClauseList> &clauses,
const T &wholeConstruct);
- void ResolveCriticalName(const parser::Name &name);
+ void ResolveCriticalName(const parser::OmpArgument &arg);
int metaLevel_{0};
const parser::OmpMetadirectiveDirective *metaDirective_{nullptr};
@@ -1961,7 +1952,7 @@ void OmpVisitor::ProcessReductionSpecifier(
}
}
-void OmpVisitor::ResolveCriticalName(const parser::Name &name) {
+void OmpVisitor::ResolveCriticalName(const parser::OmpArgument &arg) {
auto &globalScope{[&]() -> Scope & {
for (Scope *s{&currScope()};; s = &s->parent()) {
if (s->IsTopLevel()) {
@@ -1979,15 +1970,21 @@ void OmpVisitor::ResolveCriticalName(const parser::Name &name) {
}
}};
- if (auto *symbol{findSymbol(name)}) {
- if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
- SayWithDecl(name, *symbol,
- "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
- name.ToString());
+ if (auto *object{parser::Unwrap<parser::OmpObject>(arg.u)}) {
+ if (auto *desg{omp::GetDesignatorFromObj(*object)}) {
+ if (auto *name{getDesignatorNameIfDataRef(*desg)}) {
+ if (auto *symbol{findSymbol(*name)}) {
+ if (!symbol->test(Symbol::Flag::OmpCriticalLock)) {
+ SayWithDecl(*name, *symbol,
+ "CRITICAL construct name '%s' conflicts with a previous declaration"_warn_en_US,
+ name->ToString());
+ }
+ } else {
+ name->symbol = &MakeSymbol(globalScope, name->source, Attrs{});
+ name->symbol->set(Symbol::Flag::OmpCriticalLock);
+ }
+ }
}
- } else {
- name.symbol = &MakeSymbol(globalScope, name.source, Attrs{});
- name.symbol->set(Symbol::Flag::OmpCriticalLock);
}
}
diff --git a/flang/lib/Semantics/unparse-with-symbols.cpp b/flang/lib/Semantics/unparse-with-symbols.cpp
index 3093e39ba2411..41077e0e0aad7 100644
--- a...
[truncated]
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than one nit comment.
@@ -34,6 +34,7 @@ | |||
#include "flang/Parser/openmp-utils.h" | |||
#include "flang/Parser/parse-tree.h" | |||
#include "flang/Semantics/openmp-directive-sets.h" | |||
#include "flang/Semantics/openmp-utils.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is this change to the header file ___location critical for this patch? If it is not, could we do this in a separate PR? Otherwise, please ignore this comment.
This allows not having the END CRITICAL directive in certain situations. Update semantic checks and symbol resolution.