llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) <details> <summary>Changes</summary> Previously, loop constructs were parsed in a piece-wise manner: the begin directive, the body, and the end directive were parsed separately. Later on in canonicalization they were all coalesced into a loop construct. To facilitate that end-loop directives were given a special treatment, namely they were parsed as OpenMP constructs. As a result syntax errors caused by misplaced end-loop directives were handled differently from those cause by misplaced non-loop end directives. The new loop nest parser constructs the complete loop construct, removing the need for the canonicalization step. Additionally, it is the basis for parsing loop-sequence-associated constructs in the future. It also removes the need for the special treatment of end-loop directives. While this patch temporarily degrades the error messaging for misplaced end-loop directives, it enables uniform handling of any misplaced end-directives in the future. --- Patch is 36.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/168884.diff 16 Files Affected: - (modified) flang/include/flang/Parser/parse-tree.h (+1-2) - (modified) flang/lib/Parser/executable-parsers.cpp (-1) - (modified) flang/lib/Parser/openmp-parsers.cpp (+135-8) - (modified) flang/lib/Parser/parse-tree.cpp (+27-8) - (modified) flang/lib/Semantics/canonicalize-omp.cpp (-163) - (modified) flang/lib/Semantics/check-omp-loop.cpp (+74) - (modified) flang/lib/Semantics/check-omp-structure.h (+4) - (modified) flang/lib/Semantics/resolve-directives.cpp (-7) - (modified) flang/test/Lower/OpenMP/nested-loop-transformation-construct02.f90 (+1-1) - (modified) flang/test/Parser/OpenMP/loop-transformation-construct01.f90 (+4-3) - (modified) flang/test/Parser/OpenMP/loop-transformation-construct02.f90 (+4-3) - (modified) flang/test/Parser/OpenMP/tile-fail.f90 (+2-3) - (modified) flang/test/Semantics/OpenMP/do21.f90 (+5-5) - (modified) flang/test/Semantics/OpenMP/loop-association.f90 (+13-20) - (modified) flang/test/Semantics/OpenMP/loop-transformation-construct01.f90 (+22-18) - (modified) flang/test/Semantics/OpenMP/tile02.f90 (+1-1) ``````````diff diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h index 60d2ad0b764b9..9795a0d2ae25e 100644 --- a/flang/include/flang/Parser/parse-tree.h +++ b/flang/include/flang/Parser/parse-tree.h @@ -271,7 +271,6 @@ struct OpenACCRoutineConstruct; struct OpenMPConstruct; struct OpenMPLoopConstruct; struct OpenMPDeclarativeConstruct; -struct OmpEndLoopDirective; struct CUFKernelDoConstruct; // Cooked character stream locations @@ -539,7 +538,6 @@ struct ExecutableConstruct { common::Indirection<OpenACCConstruct>, common::Indirection<AccEndCombinedDirective>, common::Indirection<OpenMPConstruct>, - common::Indirection<OmpEndLoopDirective>, common::Indirection<CUFKernelDoConstruct>> u; }; @@ -5359,6 +5357,7 @@ struct OpenMPLoopConstruct { const DoConstruct *GetNestedLoop() const; const OpenMPLoopConstruct *GetNestedConstruct() const; + CharBlock source; std::tuple<OmpBeginLoopDirective, Block, std::optional<OmpEndLoopDirective>> t; }; diff --git a/flang/lib/Parser/executable-parsers.cpp b/flang/lib/Parser/executable-parsers.cpp index fadec1f11d1db..8d777a6671495 100644 --- a/flang/lib/Parser/executable-parsers.cpp +++ b/flang/lib/Parser/executable-parsers.cpp @@ -49,7 +49,6 @@ constexpr auto executableConstruct{first( construct<ExecutableConstruct>(indirect(Parser<SelectTypeConstruct>{})), construct<ExecutableConstruct>(indirect(whereConstruct)), construct<ExecutableConstruct>(indirect(forallConstruct)), - construct<ExecutableConstruct>(indirect(ompEndLoopDirective)), construct<ExecutableConstruct>(indirect(openmpConstruct)), construct<ExecutableConstruct>(indirect(Parser<OpenACCConstruct>{})), construct<ExecutableConstruct>(indirect(compilerDirective)), diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp index e2da60ed19de8..d50f45794230b 100644 --- a/flang/lib/Parser/openmp-parsers.cpp +++ b/flang/lib/Parser/openmp-parsers.cpp @@ -17,6 +17,7 @@ #include "type-parser-implementation.h" #include "flang/Parser/openmp-utils.h" #include "flang/Parser/parse-tree.h" +#include "flang/Parser/tools.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/Bitset.h" #include "llvm/ADT/STLExtras.h" @@ -30,6 +31,7 @@ #include <iterator> #include <list> #include <optional> +#include <set> #include <string> #include <tuple> #include <type_traits> @@ -1656,6 +1658,100 @@ struct LooselyStructuredBlockParser { } }; +struct NonBlockDoConstructParser { + using resultType = Block; + + std::optional<resultType> Parse(ParseState &state) const { + std::set<Label> labels; + Block body; + + // Parse nests like + // do 20 i = 1, n LabelDoStmt.t<Label> = 20 + // do 10 j = 1, m + // ... + // 10 continue Statement<...>.label = 10 + // 20 continue + + // Keep parsing ExecutionPartConstructs until the set of open label-do + // statements becomes empty, or until the EPC parser fails. + while (auto &&epc{attempt(executionPartConstruct).Parse(state)}) { + if (auto &&label{GetStatementLabel(*epc)}) { + labels.erase(*label); + } + if (auto *labelDo{Unwrap<LabelDoStmt>(*epc)}) { + labels.insert(std::get<Label>(labelDo->t)); + } + body.push_back(std::move(*epc)); + if (labels.empty()) { + break; + } + } + + if (!body.empty()) { + return std::move(body); + } + return std::nullopt; + } + +private: + // Is the template argument "Statement<T>" for some T? + template <typename T> struct IsStatement { + static constexpr bool value{false}; + }; + template <typename T> struct IsStatement<Statement<T>> { + static constexpr bool value{true}; + }; + + // Get the Label from a Statement<...> contained in an ExecutionPartConstruct, + // or std::nullopt, if there is no Statement<...> contained in there. + template <typename T> + static std::optional<Label> GetStatementLabel(const T &stmt) { + if constexpr (IsStatement<T>::value) { + return stmt.label; + } else if constexpr (WrapperTrait<T>) { + return GetStatementLabel(stmt.v); + } else if constexpr (UnionTrait<T>) { + return common::visit( + [&](auto &&s) { return GetStatementLabel(s); }, stmt.u); + } + return std::nullopt; + } +}; + +struct LoopNestParser { + using resultType = Block; + + std::optional<resultType> Parse(ParseState &state) const { + // Parse !$DIR as an ExecutionPartConstruct + auto fortranDirective{predicated(executionPartConstruct, + [](auto &epc) { return Unwrap<CompilerDirective>(epc); })}; + // Parse DO loop as an ExecutionPartConstruct + auto fortranDoConstruct{predicated(executionPartConstruct, + [&](auto &epc) { return Unwrap<DoConstruct>(epc); })}; + ParseState backtrack{state}; + + Block body; + llvm::move(*many(fortranDirective).Parse(state), std::back_inserter(body)); + + if (auto &&doLoop{attempt(fortranDoConstruct).Parse(state)}) { + body.push_back(std::move(*doLoop)); + return std::move(body); + } + if (auto &&labelDo{attempt(NonBlockDoConstructParser{}).Parse(state)}) { + llvm::move(*labelDo, std::back_inserter(body)); + return std::move(body); + } + if (auto &&sblock{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) { + llvm::move(*sblock, std::back_inserter(body)); + return std::move(body); + } + // If it's neither a DO-loop, nor a BLOCK, undo the parsing of the + // directives and fail. + state = backtrack; + return std::nullopt; + } +}; + TYPE_PARSER(construct<OmpErrorDirective>( predicated(Parser<OmpDirectiveName>{}, IsDirective(llvm::omp::Directive::OMPD_error)) >= @@ -1783,6 +1879,43 @@ struct OmpBlockConstructParser { llvm::omp::Directive dir_; }; +struct OmpLoopConstructParser { + using resultType = OpenMPLoopConstruct; + + constexpr OmpLoopConstructParser(DirectiveSet dirs) : dirs_(dirs) {} + + std::optional<resultType> Parse(ParseState &state) const { + if (auto &&begin{OmpBeginDirectiveParser(dirs_).Parse(state)}) { + if (auto &&nest{attempt(LoopNestParser{}).Parse(state)}) { + auto end{maybe(OmpEndDirectiveParser{dirs_}).Parse(state)}; + return OpenMPLoopConstruct{OmpBeginLoopDirective(std::move(*begin)), + std::move(*nest), + llvm::transformOptional(std::move(*end), + [](auto &&s) { return OmpEndLoopDirective(std::move(s)); })}; + } else { + // Parse a nested OpenMPLoopConstruct as the body. + auto ompLoopConstruct{predicated(executionPartConstruct, + [](auto &epc) { return Unwrap<OpenMPLoopConstruct>(epc); })}; + + // Allow empty body. + Block body; + if (auto &&omp{attempt(ompLoopConstruct).Parse(state)}) { + body.push_back(std::move(*omp)); + } + auto end{maybe(OmpEndDirectiveParser{dirs_}).Parse(state)}; + return OpenMPLoopConstruct{OmpBeginLoopDirective(std::move(*begin)), + std::move(body), + llvm::transformOptional(std::move(*end), + [](auto &&s) { return OmpEndLoopDirective(std::move(s)); })}; + } + } + return std::nullopt; + } + +private: + DirectiveSet dirs_; +}; + struct OmpDeclarativeAllocateParser { using resultType = OmpAllocateDirective; @@ -2266,13 +2399,7 @@ static constexpr DirectiveSet GetLoopDirectives() { return loopDirectives; } -TYPE_PARSER(sourced(construct<OmpBeginLoopDirective>( - sourced(OmpBeginDirectiveParser(GetLoopDirectives()))))) - -// END OMP Loop directives -TYPE_PARSER(sourced(construct<OmpEndLoopDirective>( - sourced(OmpEndDirectiveParser(GetLoopDirectives()))))) +TYPE_PARSER(sourced(construct<OpenMPLoopConstruct>( + OmpLoopConstructParser(GetLoopDirectives())))) -TYPE_PARSER(construct<OpenMPLoopConstruct>( - Parser<OmpBeginLoopDirective>{} / endOmpLine)) } // namespace Fortran::parser diff --git a/flang/lib/Parser/parse-tree.cpp b/flang/lib/Parser/parse-tree.cpp index 60e51895cdcea..53d4e4e680caa 100644 --- a/flang/lib/Parser/parse-tree.cpp +++ b/flang/lib/Parser/parse-tree.cpp @@ -435,17 +435,36 @@ const OmpClauseList &OmpDirectiveSpecification::Clauses() const { } const DoConstruct *OpenMPLoopConstruct::GetNestedLoop() const { - if (auto &body{std::get<Block>(t)}; !body.empty()) { - return Unwrap<DoConstruct>(body.front()); - } - return nullptr; + auto getFromBlock{[](const Block &body, auto self) -> const DoConstruct * { + for (auto &stmt : body) { + if (auto *block{Unwrap<BlockConstruct>(&stmt)}) { + return self(std::get<Block>(block->t), self); + } + if (auto *loop{Unwrap<DoConstruct>(&stmt)}) { + return loop; + } + } + return nullptr; + }}; + + return getFromBlock(std::get<Block>(t), getFromBlock); } const OpenMPLoopConstruct *OpenMPLoopConstruct::GetNestedConstruct() const { - if (auto &body{std::get<Block>(t)}; !body.empty()) { - return Unwrap<OpenMPLoopConstruct>(body.front()); - } - return nullptr; + auto getFromBlock{ + [](const Block &body, auto self) -> const OpenMPLoopConstruct * { + for (auto &stmt : body) { + if (auto *block{Unwrap<BlockConstruct>(&stmt)}) { + return self(std::get<Block>(block->t), self); + } + if (auto *omp{Unwrap<OpenMPLoopConstruct>(&stmt)}) { + return omp; + } + } + return nullptr; + }}; + + return getFromBlock(std::get<Block>(t), getFromBlock); } static bool InitCharBlocksFromStrings(llvm::MutableArrayRef<CharBlock> blocks, diff --git a/flang/lib/Semantics/canonicalize-omp.cpp b/flang/lib/Semantics/canonicalize-omp.cpp index 0cec1969e0978..8a45cc3a88f45 100644 --- a/flang/lib/Semantics/canonicalize-omp.cpp +++ b/flang/lib/Semantics/canonicalize-omp.cpp @@ -31,26 +31,6 @@ class CanonicalizationOfOmp { CanonicalizationOfOmp(SemanticsContext &context) : context_{context}, messages_{context.messages()} {} - void Post(parser::Block &block) { - for (auto it{block.begin()}; it != block.end(); ++it) { - if (auto *ompCons{GetConstructIf<parser::OpenMPConstruct>(*it)}) { - // OpenMPLoopConstruct - if (auto *ompLoop{ - std::get_if<parser::OpenMPLoopConstruct>(&ompCons->u)}) { - RewriteOpenMPLoopConstruct(*ompLoop, block, it); - } - } else if (auto *endDir{ - GetConstructIf<parser::OmpEndLoopDirective>(*it)}) { - // Unmatched OmpEndLoopDirective - const parser::OmpDirectiveName &endName{endDir->DirName()}; - messages_.Say(endName.source, - "The %s directive must follow the DO loop associated with the " - "loop construct"_err_en_US, - parser::ToUpperCaseLetters(endName.source.ToString())); - } - } // Block list - } - // Pre-visit all constructs that have both a specification part and // an execution part, and store the connection between the two. bool Pre(parser::BlockConstruct &x) { @@ -92,149 +72,6 @@ class CanonicalizationOfOmp { void Post(parser::OmpMapClause &map) { CanonicalizeMapModifiers(map); } private: - template <typename T> T *GetConstructIf(parser::ExecutionPartConstruct &x) { - if (auto *y{std::get_if<parser::ExecutableConstruct>(&x.u)}) { - if (auto *z{std::get_if<common::Indirection<T>>(&y->u)}) { - return &z->value(); - } - } - return nullptr; - } - - template <typename T> T *GetOmpIf(parser::ExecutionPartConstruct &x) { - if (auto *construct{GetConstructIf<parser::OpenMPConstruct>(x)}) { - if (auto *omp{std::get_if<T>(&construct->u)}) { - return omp; - } - } - return nullptr; - } - - void RewriteOpenMPLoopConstruct(parser::OpenMPLoopConstruct &x, - parser::Block &block, parser::Block::iterator it) { - // Check the sequence of DoConstruct and OmpEndLoopDirective - // in the same iteration - // - // Original: - // ExecutableConstruct -> OpenMPConstruct -> OpenMPLoopConstruct - // OmpBeginLoopDirective - // ExecutableConstruct -> DoConstruct - // ExecutableConstruct -> OmpEndLoopDirective (if available) - // - // After rewriting: - // ExecutableConstruct -> OpenMPConstruct -> OpenMPLoopConstruct - // OmpBeginLoopDirective - // DoConstruct - // OmpEndLoopDirective (if available) - parser::Block::iterator nextIt; - const parser::OmpDirectiveSpecification &beginDir{x.BeginDir()}; - const parser::OmpDirectiveName &beginName{beginDir.DirName()}; - - auto missingDoConstruct = [](const parser::OmpDirectiveName &dirName, - parser::Messages &messages) { - messages.Say(dirName.source, - "A DO loop must follow the %s directive"_err_en_US, - parser::ToUpperCaseLetters(dirName.source.ToString())); - }; - auto tileUnrollError = [](const parser::OmpDirectiveName &dirName, - parser::Messages &messages) { - messages.Say(dirName.source, - "If a loop construct has been fully unrolled, it cannot then be tiled"_err_en_US, - parser::ToUpperCaseLetters(dirName.source.ToString())); - }; - - auto &body{std::get<parser::Block>(x.t)}; - - nextIt = it; - while (++nextIt != block.end()) { - // Ignore compiler directives. - if (GetConstructIf<parser::CompilerDirective>(*nextIt)) - continue; - - if (auto *doCons{GetConstructIf<parser::DoConstruct>(*nextIt)}) { - if (doCons->GetLoopControl()) { - // move DoConstruct - body.push_back(std::move(*nextIt)); - nextIt = block.erase(nextIt); - // try to match OmpEndLoopDirective - if (nextIt != block.end()) { - if (auto *endDir{ - GetConstructIf<parser::OmpEndLoopDirective>(*nextIt)}) { - std::get<std::optional<parser::OmpEndLoopDirective>>(x.t) = - std::move(*endDir); - nextIt = block.erase(nextIt); - } - } - } else { - messages_.Say(beginName.source, - "DO loop after the %s directive must have loop control"_err_en_US, - parser::ToUpperCaseLetters(beginName.source.ToString())); - } - } else if (auto *ompLoopCons{ - GetOmpIf<parser::OpenMPLoopConstruct>(*nextIt)}) { - // We should allow UNROLL and TILE constructs to be inserted between an - // OpenMP Loop Construct and the DO loop itself - auto &nestedBeginDirective = ompLoopCons->BeginDir(); - auto &nestedBeginName = nestedBeginDirective.DirName(); - if ((nestedBeginName.v == llvm::omp::Directive::OMPD_unroll || - nestedBeginName.v == llvm::omp::Directive::OMPD_tile) && - !(nestedBeginName.v == llvm::omp::Directive::OMPD_unroll && - beginName.v == llvm::omp::Directive::OMPD_tile)) { - // iterate through the remaining block items to find the end directive - // for the unroll/tile directive. - parser::Block::iterator endIt; - endIt = nextIt; - while (endIt != block.end()) { - if (auto *endDir{ - GetConstructIf<parser::OmpEndLoopDirective>(*endIt)}) { - auto &endDirName = endDir->DirName(); - if (endDirName.v == beginName.v) { - std::get<std::optional<parser::OmpEndLoopDirective>>(x.t) = - std::move(*endDir); - endIt = block.erase(endIt); - continue; - } - } - ++endIt; - } - RewriteOpenMPLoopConstruct(*ompLoopCons, block, nextIt); - body.push_back(std::move(*nextIt)); - nextIt = block.erase(nextIt); - } else if (nestedBeginName.v == llvm::omp::Directive::OMPD_unroll && - beginName.v == llvm::omp::Directive::OMPD_tile) { - // if a loop has been unrolled, the user can not then tile that loop - // as it has been unrolled - const parser::OmpClauseList &unrollClauseList{ - nestedBeginDirective.Clauses()}; - if (unrollClauseList.v.empty()) { - // if the clause list is empty for an unroll construct, we assume - // the loop is being fully unrolled - tileUnrollError(beginName, messages_); - } else { - // parse the clauses for the unroll directive to find the full - // clause - for (auto &clause : unrollClauseList.v) { - if (clause.Id() == llvm::omp::OMPC_full) { - tileUnrollError(beginName, messages_); - } - } - } - } else { - messages_.Say(nestedBeginName.source, - "Only Loop Transformation Constructs or Loop Nests can be nested within Loop Constructs"_err_en_US, - parser::ToUpperCaseLetters(nestedBeginName.source.ToString())); - } - } else { - missingDoConstruct(beginName, messages_); - } - // If we get here, we either found a loop, or issued an error message. - return; - } - if (nextIt == block.end()) { - missingDoConstruct(beginName, messages_); - } - } - // Canonicalization of allocate directives // // In OpenMP 5.0 and 5.1 the allocate directive could either be a declarative diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp index 3d3596b500880..9798420eb8086 100644 --- a/flang/lib/Semantics/check-omp-loop.cpp +++ b/flang/lib/Semantics/check-omp-loop.cpp @@ -245,6 +245,78 @@ void OmpStructureChecker::CheckSIMDNest(const parser::OpenMPConstruct &c) { } } +static bool IsLoopTransforming(llvm::omp::Directive dir) { + switch (dir) { + // TODO case llvm::omp::Directive::OMPD_flatten: + case llvm::omp::Directive::OMPD_fuse: + case llvm::omp::Directive::OMPD_interchange: + case llvm::omp::Directive::OMPD_nothing: + case llvm::omp::Directive::OMPD_reverse: + // TODO case llvm::omp::Directive::OMPD_split: + case llvm::omp::Directive::OMPD_stripe: + case llvm::omp::Directive::OMPD_tile: + case llvm::omp::Directive::OMPD_unroll: + return true; + default: + return false; + } +} + +void OmpStructureChecker::CheckNestedBlock(const parser::OpenMPLoopConstruct &x, + const parser::Block &body, size_t &nestedCount) { + for (auto &stmt : body) { + if (auto *dir{parser::Unwrap<parser::CompilerDirective>(stmt)}) { + context_.Say(dir->source, + "Compiler directives are not allowed inside OpenMP loop constructs"_err_en_US); + } else if (parser::Unwrap<parser::DoConstruct>(stmt)) { + ++nestedCount; + } else if (auto *omp{parser::Unwrap<parser::OpenMPLoopConstruct>(stmt)}) { + if (!IsLoopTransforming(omp->BeginDir().DirName().v)) { + context_.Say(omp->source, + "Only loop-transforming OpenMP constructs are allowed inside OpenMP loop constructs"_err_en_US); + } + ++nestedCount; + } else if (auto *block{parser::Unwrap<parser::BlockConstruct>(stmt)}) { + CheckNestedBlock(x, std::get<parser::Block>(block->t), nestedCount); + } else { + parser::CharBlock source{parser::GetSource(stmt).value_or(x.source)}; + context_.Say(source, + "OpenMP loop construct can only contain DO loops or loop-nest-generating OpenMP constructs"_err_en_US); + } + } +} + +void OmpStructureChecker::CheckNestedConstruct( + const parser::OpenMPLoopConstruct &x) { + size_t nestedCount{0}; + + auto &body{std::get<parser::Block>(x.t)}; + if (body.empty()) { + context_.Say(x.source, + "OpenMP loop construct should contain a DO-loop or a loop-nest-generating OpenMP construct"_err_en_US); + } else { + CheckNestedBlock(x, body, nestedCount); + } +} + +void OmpStructureChecker::CheckFullUnroll( + const parser::OpenMPLoopConstruct &x) { + // If the nested construct is a full unroll, then this construct is invalid + // since it won't contain a loop. + if (const parser::OpenMPLoopConstruct *nested{x.GetNestedConstruct()}) { + auto &nestedSpec{nested->BeginDir()}; + if (nestedSpec.DirName().v == llvm::omp::Directive::OMPD_unroll) { + bool isPartial{ + llvm::any_of(nestedSpec.Clauses().v, [](const parser::OmpClause &c) { + return c.Id() == ll... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/168884 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
