llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-parser Author: Krzysztof Parzyszek (kparzysz) <details> <summary>Changes</summary> This 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: - (modified) flang/include/flang/Parser/parse-tree.h (+3-3) - (renamed) flang/include/flang/Semantics/openmp-utils.h () - (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+18-6) - (modified) flang/lib/Parser/openmp-parsers.cpp (+2-11) - (modified) flang/lib/Parser/unparse.cpp (+1-3) - (modified) flang/lib/Semantics/check-omp-atomic.cpp (+1-1) - (modified) flang/lib/Semantics/check-omp-loop.cpp (+1-1) - (modified) flang/lib/Semantics/check-omp-metadirective.cpp (+1-2) - (modified) flang/lib/Semantics/check-omp-structure.cpp (+81-43) - (modified) flang/lib/Semantics/openmp-utils.cpp (+1-1) - (modified) flang/lib/Semantics/resolve-directives.cpp (+3-3) - (modified) flang/lib/Semantics/resolve-names.cpp (+33-36) - (modified) flang/lib/Semantics/unparse-with-symbols.cpp (-14) - (modified) flang/test/Parser/OpenMP/critical-unparse-with-symbols.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/sync-critical01.f90 (+4-4) - (modified) flang/test/Semantics/OpenMP/sync-critical02.f90 (+4-4) ``````````diff 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] `````````` </details> https://github.com/llvm/llvm-project/pull/152007 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
