llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) <details> <summary>Changes</summary> This implements checks of the validity of context set selectors and trait selectors, plus the types of trait properties. Clause properties are also validated, but not name or extension properties. --- Patch is 33.94 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123243.diff 12 Files Affected: - (modified) flang/include/flang/Parser/parse-tree.h (+2) - (modified) flang/include/flang/Semantics/openmp-modifiers.h (+1) - (modified) flang/lib/Parser/parse-tree.cpp (+20) - (modified) flang/lib/Semantics/check-omp-structure.cpp (+502-2) - (modified) flang/lib/Semantics/check-omp-structure.h (+26) - (modified) flang/lib/Semantics/openmp-modifiers.cpp (+17) - (modified) flang/test/Parser/OpenMP/metadirective.f90 (+7-7) - (added) flang/test/Semantics/OpenMP/metadirective-common.f90 (+37) - (added) flang/test/Semantics/OpenMP/metadirective-construct.f90 (+33) - (added) flang/test/Semantics/OpenMP/metadirective-device.f90 (+36) - (added) flang/test/Semantics/OpenMP/metadirective-implementation.f90 (+33) - (added) flang/test/Semantics/OpenMP/metadirective-user.f90 (+29) ``````````diff diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h index 71afb6d2ae75a7..6053ad5dc0f7ad 100644 --- a/flang/include/flang/Parser/parse-tree.h +++ b/flang/include/flang/Parser/parse-tree.h @@ -3569,6 +3569,7 @@ struct OmpTraitProperty { // Trait-set-selectors: // [D]evice, [T]arget_device, [C]onstruct, [I]mplementation, [U]ser. struct OmpTraitSelectorName { + std::string ToString() const; CharBlock source; UNION_CLASS_BOILERPLATE(OmpTraitSelectorName); ENUM_CLASS(Value, Arch, Atomic_Default_Mem_Order, Condition, Device_Num, @@ -3593,6 +3594,7 @@ struct OmpTraitSelector { // CONSTRUCT | DEVICE | IMPLEMENTATION | USER | // since 5.0 // TARGET_DEVICE // since 5.1 struct OmpTraitSetSelectorName { + std::string ToString() const; CharBlock source; ENUM_CLASS(Value, Construct, Device, Implementation, Target_Device, User) WRAPPER_CLASS_BOILERPLATE(OmpTraitSetSelectorName, Value); diff --git a/flang/include/flang/Semantics/openmp-modifiers.h b/flang/include/flang/Semantics/openmp-modifiers.h index 5d5c5e97faf413..7cdbf65adebe1c 100644 --- a/flang/include/flang/Semantics/openmp-modifiers.h +++ b/flang/include/flang/Semantics/openmp-modifiers.h @@ -72,6 +72,7 @@ DECLARE_DESCRIPTOR(parser::OmpAlignModifier); DECLARE_DESCRIPTOR(parser::OmpAllocatorComplexModifier); DECLARE_DESCRIPTOR(parser::OmpAllocatorSimpleModifier); DECLARE_DESCRIPTOR(parser::OmpChunkModifier); +DECLARE_DESCRIPTOR(parser::OmpContextSelector); DECLARE_DESCRIPTOR(parser::OmpDependenceType); DECLARE_DESCRIPTOR(parser::OmpDeviceModifier); DECLARE_DESCRIPTOR(parser::OmpDirectiveNameModifier); diff --git a/flang/lib/Parser/parse-tree.cpp b/flang/lib/Parser/parse-tree.cpp index a414f226058e3e..251b6919cf52fe 100644 --- a/flang/lib/Parser/parse-tree.cpp +++ b/flang/lib/Parser/parse-tree.cpp @@ -281,6 +281,26 @@ OmpTaskDependenceType::Value OmpDependClause::TaskDep::GetTaskDepType() const { } } +std::string OmpTraitSelectorName::ToString() const { + return common::visit( // + common::visitors{ + [&](Value v) { // + return std::string(EnumToString(v)); + }, + [&](llvm::omp::Directive d) { + return llvm::omp::getOpenMPDirectiveName(d).str(); + }, + [&](const std::string &s) { // + return s; + }, + }, + u); +} + +std::string OmpTraitSetSelectorName::ToString() const { + return std::string(EnumToString(v)); +} + } // namespace Fortran::parser template <typename C> static llvm::omp::Clause getClauseIdForClass(C &&) { diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp index d732233388d4fe..94886d6b9dfdc9 100644 --- a/flang/lib/Semantics/check-omp-structure.cpp +++ b/flang/lib/Semantics/check-omp-structure.cpp @@ -9,6 +9,8 @@ #include "check-omp-structure.h" #include "definable.h" #include "flang/Evaluate/check-expression.h" +#include "flang/Evaluate/expression.h" +#include "flang/Evaluate/type.h" #include "flang/Parser/parse-tree.h" #include "flang/Semantics/expression.h" #include "flang/Semantics/openmp-modifiers.h" @@ -2921,7 +2923,6 @@ CHECK_SIMPLE_CLAUSE(Severity, OMPC_severity) CHECK_SIMPLE_CLAUSE(Message, OMPC_message) CHECK_SIMPLE_CLAUSE(Filter, OMPC_filter) CHECK_SIMPLE_CLAUSE(Otherwise, OMPC_otherwise) -CHECK_SIMPLE_CLAUSE(When, OMPC_when) CHECK_SIMPLE_CLAUSE(AdjustArgs, OMPC_adjust_args) CHECK_SIMPLE_CLAUSE(AppendArgs, OMPC_append_args) CHECK_SIMPLE_CLAUSE(MemoryOrder, OMPC_memory_order) @@ -4468,14 +4469,513 @@ void OmpStructureChecker::Enter(const parser::OmpClause::OmpxBare &x) { } } -void OmpStructureChecker::Enter(const parser::OmpContextSelector &ctxSel) { +void OmpStructureChecker::Enter(const parser::OmpClause::When &x) { + CheckAllowedClause(llvm::omp::Clause::OMPC_when); + OmpVerifyModifiers( + x.v, llvm::omp::OMPC_when, GetContext().clauseSource, context_); +} + +void OmpStructureChecker::Enter(const parser::OmpContextSelector &ctx) { EnterDirectiveNest(ContextSelectorNest); + + using SetName = parser::OmpTraitSetSelectorName; + std::map<SetName::Value, const SetName *> visited; + + for (const parser::OmpTraitSetSelector &traitSet : ctx.v) { + auto &name{std::get<SetName>(traitSet.t)}; + auto [prev, unique]{visited.insert(std::make_pair(name.v, &name))}; + if (!unique) { + std::string showName{parser::ToUpperCaseLetters(name.ToString())}; + parser::MessageFormattedText txt( + "Repeated trait set name %s in a context specifier"_err_en_US, + showName); + parser::Message message(name.source, txt); + message.Attach(prev->second->source, + "Previous trait set %s provided here"_en_US, showName); + context_.Say(std::move(message)); + } + CheckTraitSetSelector(traitSet); + } } void OmpStructureChecker::Leave(const parser::OmpContextSelector &) { ExitDirectiveNest(ContextSelectorNest); } +std::optional<evaluate::DynamicType> OmpStructureChecker::GetDynamicType( + const common::Indirection<parser::Expr> &parserExpr) { + // Indirection<parser::Expr> parserExpr + // `- parser::Expr ^.value() + const parser::TypedExpr &typedExpr{parserExpr.value().typedExpr}; + // ForwardOwningPointer typedExpr + // `- GenericExprWrapper ^.get() + // `- std::optional<Expr> ^->v + if (auto maybeExpr{typedExpr.get()->v}) { + return maybeExpr->GetType(); + } else { + return std::nullopt; + } +} + +const std::list<parser::OmpTraitProperty> & +OmpStructureChecker::GetTraitPropertyList( + const parser::OmpTraitSelector &trait) { + static const std::list<parser::OmpTraitProperty> empty{}; + auto &[_, maybeProps]{trait.t}; + if (maybeProps) { + using PropertyList = std::list<parser::OmpTraitProperty>; + return std::get<PropertyList>(maybeProps->t); + } else { + return empty; + } +} + +std::optional<llvm::omp::Clause> OmpStructureChecker::GetClauseFromProperty( + const parser::OmpTraitProperty &property) { + using MaybeClause = std::optional<llvm::omp::Clause>; + + // The parser for OmpClause will only succeed if the clause was + // given with all required arguments. + // If this is a string or complex extensiom with a clause name, + // treat it as a clause and let the trait checker deal with it. + + auto getClauseFromString{[&](const std::string &s) -> MaybeClause { + auto id{llvm::omp::getOpenMPClauseKind(parser::ToLowerCaseLetters(s))}; + if (id != llvm::omp::Clause::OMPC_unknown) { + return id; + } else { + return std::nullopt; + } + }}; + + return common::visit( // + common::visitors{ + [&](const parser::OmpTraitPropertyName &x) -> MaybeClause { + return getClauseFromString(x.v); + }, + [&](const common::Indirection<parser::OmpClause> &x) -> MaybeClause { + return x.value().Id(); + }, + [&](const parser::ScalarExpr &x) -> MaybeClause { + return std::nullopt; + }, + [&](const parser::OmpTraitPropertyExtension &x) -> MaybeClause { + using ExtProperty = parser::OmpTraitPropertyExtension; + if (auto *name{std::get_if<parser::OmpTraitPropertyName>(&x.u)}) { + return getClauseFromString(name->v); + } else if (auto *cpx{std::get_if<ExtProperty::Complex>(&x.u)}) { + return getClauseFromString( + std::get<parser::OmpTraitPropertyName>(cpx->t).v); + } + return std::nullopt; + }, + }, + property.u); +} + +void OmpStructureChecker::CheckTraitSelectorList( + const std::list<parser::OmpTraitSelector> &traits) { + // [6.0:322:20] + // Each trait-selector-name may only be specified once in a trait selector + // set. + + // Cannot store OmpTraitSelectorName directly, because it's not copyable. + using TraitName = parser::OmpTraitSelectorName; + using BareName = decltype(TraitName::u); + std::map<BareName, const TraitName *> visited; + + for (const parser::OmpTraitSelector &trait : traits) { + auto &name{std::get<TraitName>(trait.t)}; + + auto [prev, unique]{visited.insert(std::make_pair(name.u, &name))}; + if (!unique) { + std::string showName{parser::ToUpperCaseLetters(name.ToString())}; + parser::MessageFormattedText txt( + "Repeated trait name %s in a trait set"_err_en_US, showName); + parser::Message message(name.source, txt); + message.Attach(prev->second->source, + "Previous trait %s provided here"_en_US, showName); + context_.Say(std::move(message)); + } + } +} + +void OmpStructureChecker::CheckTraitSetSelector( + const parser::OmpTraitSetSelector &traitSet) { + + // Trait Set | Allowed traits | D-traits | X-traits | Score | + // + // Construct | Simd, directive-name | Yes | No | No | + // Device | Arch, Isa, Kind | No | Yes | No | + // Implementation | Atomic_Default_Mem_Order | No | Yes | Yes | + // | Extension, Requires | | | | + // | Vendor | | | | + // Target_Device | Arch, Device_Num, Isa | No | Yes | No | + // | Kind, Uid | | | | + // User | Condition | No | No | Yes | + + struct TraitSetConfig { + std::set<parser::OmpTraitSelectorName::Value> allowed; + bool allowsDirectiveTraits; + bool allowsExtensionTraits; + bool allowsScore; + }; + + using SName = parser::OmpTraitSetSelectorName::Value; + using TName = parser::OmpTraitSelectorName::Value; + + static const std::map<SName, TraitSetConfig> configs{ + {SName::Construct, // + {{TName::Simd}, true, false, false}}, + {SName::Device, // + {{TName::Arch, TName::Isa, TName::Kind}, false, true, false}}, + {SName::Implementation, // + {{TName::Atomic_Default_Mem_Order, TName::Extension, TName::Requires, + TName::Vendor}, + false, true, true}}, + {SName::Target_Device, // + {{TName::Arch, TName::Device_Num, TName::Isa, TName::Kind, + TName::Uid}, + false, true, false}}, + {SName::User, // + {{TName::Condition}, false, false, true}}, + }; + + auto checkTraitSet{[&](const TraitSetConfig &config) { + auto &[setName, traits]{traitSet.t}; + auto usn{parser::ToUpperCaseLetters(setName.ToString())}; + + // Check if there are any duplicate traits. + CheckTraitSelectorList(traits); + + for (const parser::OmpTraitSelector &trait : traits) { + auto &[traitName, maybeProps]{trait.t}; + + // Check allowed traits + common::visit( // + common::visitors{ + [&](parser::OmpTraitSelectorName::Value v) { + if (!config.allowed.count(v)) { + context_.Say(traitName.source, + "%s is not a valid trait for %s trait set"_err_en_US, + parser::ToUpperCaseLetters(traitName.ToString()), usn); + } + }, + [&](llvm::omp::Directive) { + if (!config.allowsDirectiveTraits) { + context_.Say(traitName.source, + "Directive name is not a valid trait for %s trait set"_err_en_US, + usn); + } + }, + [&](const std::string &) { + if (!config.allowsExtensionTraits) { + context_.Say(traitName.source, + "Extension traits are not valid for %s trait set"_err_en_US, + usn); + } + }, + }, + traitName.u); + + // Check score + if (maybeProps) { + auto &[maybeScore, _]{maybeProps->t}; + if (maybeScore) { + CheckTraitScore(*maybeScore); + } + } + + // Check the properties of the individual traits + CheckTraitSelector(traitSet, trait); + } + }}; + + checkTraitSet( + configs.at(std::get<parser::OmpTraitSetSelectorName>(traitSet.t).v)); +} + +void OmpStructureChecker::CheckTraitScore(const parser::OmpTraitScore &score) { + // [6.0:322:23] + // A score-expression must be a non-negative constant integer expression. + if (auto value{GetIntValue(score)}; !value || value <= 0) { + context_.Say(score.source, + "SCORE expression must be a non-negative constant integer expression"_err_en_US); + } +} + +bool OmpStructureChecker::VerifyTraitPropertyLists( + const parser::OmpTraitSetSelector &traitSet, + const parser::OmpTraitSelector &trait) { + using TraitName = parser::OmpTraitSelectorName; + using PropertyList = std::list<parser::OmpTraitProperty>; + auto &[traitName, maybeProps]{trait.t}; + + auto checkPropertyList{[&](const PropertyList &properties, auto isValid, + const std::string &message) { + bool foundInvalid{false}; + for (const parser::OmpTraitProperty &prop : properties) { + if (!isValid(prop)) { + if (foundInvalid) { + context_.Say( + prop.source, "More invalid properties are present"_err_en_US); + break; + } + context_.Say(prop.source, "%s"_err_en_US, message); + foundInvalid = true; + } + } + return !foundInvalid; + }}; + + bool invalid{false}; + + if (std::holds_alternative<llvm::omp::Directive>(traitName.u)) { + // Directive-name traits don't have properties. + if (maybeProps) { + context_.Say(trait.source, + "Directive-name traits cannot have properties"_err_en_US); + invalid = true; + } + } + // Ignore properties on extension traits. + + // See `TraitSelectorParser` in openmp-parser.cpp + if (auto *v{std::get_if<TraitName::Value>(&traitName.u)}) { + switch (*v) { + // name-list properties + case parser::OmpTraitSelectorName::Value::Arch: + case parser::OmpTraitSelectorName::Value::Extension: + case parser::OmpTraitSelectorName::Value::Isa: + case parser::OmpTraitSelectorName::Value::Kind: + case parser::OmpTraitSelectorName::Value::Uid: + case parser::OmpTraitSelectorName::Value::Vendor: + if (maybeProps) { + auto isName{[](const parser::OmpTraitProperty &prop) { + return std::holds_alternative<parser::OmpTraitPropertyName>(prop.u); + }}; + invalid = !checkPropertyList(std::get<PropertyList>(maybeProps->t), + isName, "Trait property should be a name"); + } + break; + // clause-list + case parser::OmpTraitSelectorName::Value::Atomic_Default_Mem_Order: + case parser::OmpTraitSelectorName::Value::Requires: + case parser::OmpTraitSelectorName::Value::Simd: + if (maybeProps) { + auto isClause{[&](const parser::OmpTraitProperty &prop) { + return GetClauseFromProperty(prop).has_value(); + }}; + invalid = !checkPropertyList(std::get<PropertyList>(maybeProps->t), + isClause, "Trait property should be a clause"); + } + break; + // expr-list + case parser::OmpTraitSelectorName::Value::Condition: + case parser::OmpTraitSelectorName::Value::Device_Num: + if (maybeProps) { + auto isExpr{[](const parser::OmpTraitProperty &prop) { + return std::holds_alternative<parser::ScalarExpr>(prop.u); + }}; + invalid = !checkPropertyList(std::get<PropertyList>(maybeProps->t), + isExpr, "Trait property should be a scalar expression"); + } + break; + } // switch + } + + return !invalid; +} + +void OmpStructureChecker::CheckTraitSelector( + const parser::OmpTraitSetSelector &traitSet, + const parser::OmpTraitSelector &trait) { + using TraitName = parser::OmpTraitSelectorName; + auto &[traitName, maybeProps]{trait.t}; + + // Only do the detailed checks if the property lists are valid. + if (VerifyTraitPropertyLists(traitSet, trait)) { + if (std::holds_alternative<llvm::omp::Directive>(traitName.u) || + std::holds_alternative<std::string>(traitName.u)) { + // No properties here: directives don't have properties, and + // we don't implement any extension traits now. + return; + } + + // Specific traits we want to check. + // Limitations: + // (1) The properties for these traits are defined in "Additional + // Definitions for the OpenMP API Specification". It's not clear how + // to define them in a portable way, and how to verify their validity, + // especially if they get replaced by their integer values (in case + // they are defined as enums). + // (2) These are entirely implementation-defined, and at the moment + // there is no known schema to validate these values. + auto v{std::get<TraitName::Value>(traitName.u)}; + switch (v) { + case TraitName::Value::Arch: + // Unchecked, TBD(1) + break; + case TraitName::Value::Atomic_Default_Mem_Order: + CheckTraitADMO(traitSet, trait); + break; + case TraitName::Value::Condition: + CheckTraitCondition(traitSet, trait); + break; + case TraitName::Value::Device_Num: + CheckTraitDeviceNum(traitSet, trait); + break; + case TraitName::Value::Extension: + // Ignore + break; + case TraitName::Value::Isa: + // Unchecked, TBD(1) + break; + case TraitName::Value::Kind: + // Unchecked, TBD(1) + break; + case TraitName::Value::Requires: + CheckTraitRequires(traitSet, trait); + break; + case TraitName::Value::Simd: + CheckTraitSimd(traitSet, trait); + break; + case TraitName::Value::Uid: + // Unchecked, TBD(2) + break; + case TraitName::Value::Vendor: + // Unchecked, TBD(1) + break; + } + } +} + +void OmpStructureChecker::CheckTraitADMO( + const parser::OmpTraitSetSelector &traitSet, + const parser::OmpTraitSelector &trait) { + auto &traitName{std::get<parser::OmpTraitSelectorName>(trait.t)}; + auto &properties{GetTraitPropertyList(trait)}; + + if (properties.size() != 1) { + context_.Say(trait.source, + "%s trait requires a single clause property"_err_en_US, + parser::ToUpperCaseLetters(traitName.ToString())); + } else { + const parser::OmpTraitProperty &property{properties.front()}; + auto clauseId{*GetClauseFromProperty(property)}; + // Check that the clause belongs to the memory-order clause-set. + // Clause sets will hopefully be autogenerated at some point. + switch (clauseId) { + case llvm::omp::Clause::OMPC_acq_rel: + case llvm::omp::Clause::OMPC_acquire: + case llvm::omp::Clause::OMPC_relaxed: + case llvm::omp::Clause::OMPC_release: + case llvm::omp::Clause::OMPC_seq_cst: + break; + default: + context_.Say(property.source, + "%s trait requires a clause from the memory-order clause set"_err_en_US, + parser::ToUpperCaseLetters(traitName.ToString())); + } + + using ClauseProperty = common::Indirection<parser::OmpClause>; + if (!std::holds_alternative<ClauseProperty>(property.u)) { + context_.Say(property.source, + "Invalid clause specification for %s"_err_en_US, + parser::ToUpperCaseLetters(getClauseName(clauseId))); + } + } +} + +void OmpStructureChecker::CheckTraitCondition( + const parser::OmpTraitSetSelector &traitSet, + const parser::OmpTraitSelector &trait) { + auto &traitName{std::get<parser::OmpTraitSelectorName>(trait.t)}; + auto &properties{GetTraitPropertyList(trait)}; + + if (properties.size() != 1) { + context_.Say(trait.source, + "%s trait requires a single expression property"_err_en_US, + parser::ToUpperCaseLetters(traitName.ToString())); + } else { + const parser::OmpTraitProperty &property{properties.front()}; + auto &scalarExpr{std::get<parser::ScalarExpr>(property.u)}; + + auto maybeType{GetDynamicType(scalarExpr.thing)}; + if (!maybeType || maybeType->category() != TypeCategory::Logical) { + context_.Say(property.source, + "%s trait requires a single LOGICAL expression"_err_en_US, + parser::ToUpperCaseLetters(traitName.ToString())); + } + } +} + +void OmpStructureChecker::CheckTrait... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/123243 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
