llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) <details> <summary>Changes</summary> Check if the code associated with a nest or sequence construct is well formed. Emit diagnostic messages if not. Make a clearer separation for checks of loop-nest-associated and loop- sequence-associated constructs. Unify structure of some of the more common messages. Issue: https://github.com/llvm/llvm-project/issues/185287 --- Patch is 41.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/188025.diff 23 Files Affected: - (modified) flang/include/flang/Semantics/openmp-utils.h (+3) - (modified) flang/lib/Semantics/check-omp-loop.cpp (+53-73) - (modified) flang/lib/Semantics/openmp-utils.cpp (+115-47) - (modified) flang/lib/Semantics/resolve-directives.cpp (-5) - (removed) flang/test/Parser/OpenMP/interchange-fail.f90 (-31) - (removed) flang/test/Parser/OpenMP/tile-fail.f90 (-31) - (modified) flang/test/Semantics/OpenMP/do-collapse.f90 (+1-2) - (modified) flang/test/Semantics/OpenMP/do-concurrent-collapse.f90 (+6-5) - (modified) flang/test/Semantics/OpenMP/do09.f90 (+4-2) - (modified) flang/test/Semantics/OpenMP/do13.f90 (+5-5) - (modified) flang/test/Semantics/OpenMP/do21.f90 (+5-5) - (modified) flang/test/Semantics/OpenMP/fuse1.f90 (+1-1) - (added) flang/test/Semantics/OpenMP/interchange-fail.f90 (+18) - (modified) flang/test/Semantics/OpenMP/interchange01.f90 (+5-3) - (modified) flang/test/Semantics/OpenMP/loop-association.f90 (+9-6) - (modified) flang/test/Semantics/OpenMP/loop-transformation-construct01.f90 (+10-7) - (modified) flang/test/Semantics/OpenMP/loop-transformation-construct02.f90 (+6-5) - (modified) flang/test/Semantics/OpenMP/loop-transformation-construct04.f90 (+2-2) - (added) flang/test/Semantics/OpenMP/tile-fail.f90 (+18) - (modified) flang/test/Semantics/OpenMP/tile02.f90 (+2-1) - (modified) flang/test/Semantics/OpenMP/tile03.f90 (+2-1) - (modified) flang/test/Semantics/OpenMP/tile08.f90 (+2-2) - (modified) flang/test/Semantics/OpenMP/tile09.f90 (+2-1) ``````````diff diff --git a/flang/include/flang/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h index 7c95edf81ada2..bf35598177e17 100644 --- a/flang/include/flang/Semantics/openmp-utils.h +++ b/flang/include/flang/Semantics/openmp-utils.h @@ -202,6 +202,9 @@ struct LoopSequence { const Depth &depth() const { return depth_; } const std::vector<LoopSequence> &children() const { return children_; } + WithReason<bool> isWellFormedSequence() const; + WithReason<bool> isWellFormedNest() const; + private: using Construct = ExecutionPartIterator::Construct; diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp index df9797ac8e56a..962537496e580 100644 --- a/flang/lib/Semantics/check-omp-loop.cpp +++ b/flang/lib/Semantics/check-omp-loop.cpp @@ -61,16 +61,6 @@ class AssociatedLoopChecker { constructNamesAndLevels_.emplace( constructName.value().ToString(), level_); } - if (level_ >= 0) { - if (dc.IsDoWhile()) { - context_.Say(doStmt.source, - "The associated loop of a loop-associated directive cannot be a DO WHILE."_err_en_US); - } - if (!dc.GetLoopControl()) { - context_.Say(doStmt.source, - "The associated loop of a loop-associated directive cannot be a DO without control."_err_en_US); - } - } return true; } @@ -268,93 +258,83 @@ void OmpStructureChecker::CheckNestedConstruct( // Check constructs contained in the body of the loop construct. auto &body{std::get<parser::Block>(x.t)}; + for (auto &stmt : BlockRange(body, BlockRange::Step::Over)) { if (auto *d{parser::Unwrap<parser::CompilerDirective>(stmt)}) { context_.Say(d->source, "Compiler directives are not allowed inside OpenMP loop constructs"_warn_en_US); - } else if (auto *omp{parser::Unwrap<parser::OpenMPLoopConstruct>(stmt)}) { - if (!IsLoopTransforming(omp->BeginDir().DirId())) { - context_.Say(omp->source, - "Only loop-transforming OpenMP constructs are allowed inside OpenMP loop constructs"_err_en_US); - } - if (IsFullUnroll(*omp)) { - context_.Say(x.source, - "OpenMP loop construct cannot apply to a fully unrolled loop"_err_en_US); - } - } else if (!parser::Unwrap<parser::DoConstruct>(stmt)) { - 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); } } LoopSequence sequence(body, version, true); - // Check if a loop-nest-associated construct has only one top-level loop - // in it. + auto assoc{llvm::omp::getDirectiveAssociation(dir)}; auto needRange{GetAffectedLoopRangeWithReason(beginSpec, version)}; + auto haveLength{sequence.length()}; - if (auto haveLength{sequence.length()}) { - if (*haveLength.value == 0) { + if (assoc == llvm::omp::Association::LoopNest) { + if (sequence.children().size() == 0) { context_.Say(beginSource, - "This construct should contain a DO-loop or a loop-nest-generating OpenMP construct"_err_en_US); - } else { - auto assoc{llvm::omp::getDirectiveAssociation(dir)}; - if (*haveLength.value > 1 && assoc == llvm::omp::Association::LoopNest) { - auto &msg{context_.Say(beginSource, - "This construct applies to a loop nest, but has a loop sequence of " - "length %" PRId64 ""_err_en_US, - *haveLength.value)}; - haveLength.reason.AttachTo(msg); - } - if (assoc == llvm::omp::Association::LoopSeq) { - if (auto requiredCount{GetRequiredCount(needRange.value)}) { - if (*requiredCount > 0 && *haveLength.value < *requiredCount) { - auto &msg{context_.Say(beginSource, - "This construct requires a sequence of %" PRId64 - " loops, but the loop sequence has a length of %" PRId64 - ""_err_en_US, - *requiredCount, *haveLength.value)}; - haveLength.reason.AttachTo(msg); - needRange.reason.AttachTo(msg); - } - } - } + "This construct should contain a DO-loop or a loop-nest-generating construct"_err_en_US); + } else if (haveLength.value > 1) { + auto &msg{context_.Say(beginSource, + "This construct applies to a loop nest, but has a loop sequence of " + "length %" PRId64 ""_err_en_US, + *haveLength.value)}; + haveLength.reason.AttachTo(msg); + } + auto [isWellFormed, whyNot]{sequence.isWellFormedNest()}; + if (isWellFormed && !*isWellFormed) { + auto &msg{context_.Say(beginSource, + "This construct requires a canonical loop nest"_err_en_US)}; + whyNot.AttachTo(msg); } - } - // Check requirements on nest depth. - auto [needDepth, needPerfect]{ - GetAffectedNestDepthWithReason(beginSpec, version)}; - auto &[haveSema, havePerf]{sequence.depth()}; + // Check requirements on nest depth. + auto [needDepth, needPerfect]{ + GetAffectedNestDepthWithReason(beginSpec, version)}; + auto &[haveSema, havePerf]{sequence.depth()}; + + auto haveDepth{needPerfect ? havePerf : haveSema}; + std::string_view perfectTxt{needPerfect ? " perfect" : ""}; - if (dir != llvm::omp::Directive::OMPD_fuse) { - auto haveDepth = needPerfect ? havePerf : haveSema; // If the present depth is 0, it's likely that the construct doesn't // have any loops in it, which would be diagnosed above. if (needDepth && haveDepth.value > 0) { if (*needDepth.value > *haveDepth.value) { - if (needPerfect) { + auto &msg{context_.Say(beginSource, + "This construct requires a%s nest of depth %" PRId64 + ", but the associated nest is a%s nest of depth %" PRId64 + ""_err_en_US, + perfectTxt, *needDepth.value, perfectTxt, *haveDepth.value)}; + haveDepth.reason.AttachTo(msg); + needDepth.reason.AttachTo(msg); + } + } + + } else if (assoc == llvm::omp::Association::LoopSeq) { + if (haveLength.value == 0) { + context_.Say(beginSource, + "This construct should contain a DO-loop or a loop-sequence-generating construct"_err_en_US); + } else { + auto [isWellFormed, whyNot]{sequence.isWellFormedSequence()}; + if (isWellFormed && !*isWellFormed) { + auto &msg{context_.Say(beginSource, + "This construct requires a canonical loop sequence"_err_en_US)}; + whyNot.AttachTo(msg); + } + if (auto requiredCount{GetRequiredCount(needRange.value)}) { + if (*requiredCount > 0 && haveLength.value < *requiredCount) { auto &msg{context_.Say(beginSource, - "This construct requires a perfect nest of depth %" PRId64 - ", but the associated nest is a perfect nest of depth %" PRId64 + "This construct requires a sequence of %" PRId64 + " loops, but the loop sequence has a length of %" PRId64 ""_err_en_US, - *needDepth.value, *haveDepth.value)}; - haveDepth.reason.AttachTo(msg); - needDepth.reason.AttachTo(msg); - } else { - auto &msg{context_.Say(beginSource, - "This construct requires a nest of depth %" PRId64 - ", but the associated nest has a depth of %" PRId64 ""_err_en_US, - *needDepth.value, *haveDepth.value)}; - haveDepth.reason.AttachTo(msg); - needDepth.reason.AttachTo(msg); + *requiredCount, *haveLength.value)}; + haveLength.reason.AttachTo(msg); + needRange.reason.AttachTo(msg); } } } - } else { - // FUSE requires a sequence of perfect nests. - // TODO: Defer this check for now. } } diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp index 20a4de77a1d7f..d9fe3c4a5d02f 100644 --- a/flang/lib/Semantics/openmp-utils.cpp +++ b/flang/lib/Semantics/openmp-utils.cpp @@ -532,6 +532,13 @@ MaybeExpr MakeEvaluateExpr(const parser::OmpStylizedInstance &inp) { instance.u); } +static const auto MsgNotValidAffectedLoop{ + "%s is not a valid affected loop"_because_en_US}; +static const auto MsgClauseAbsentAssume{ + "%s clause was not specified, %s is assumed"_because_en_US}; +static const auto MsgConstructDoesNotResult{ + "%s does not result in %s"_because_en_US}; + Reason::Reason(const Reason &other) { // CopyFrom(other); } @@ -565,7 +572,7 @@ WithReason<int64_t> GetArgumentValueWithReason( Reason reason; reason.Say(clause->source, "%s clause was specified with argument %" PRId64 ""_because_en_US, - name.c_str(), *value); + name, *value); return {*value, std::move(reason)}; } } @@ -581,7 +588,7 @@ static WithReason<int64_t> GetNumArgumentsWithReasonForType( Reason reason; reason.Say(clause.source, "%s clause was specified with %" PRId64 " arguments"_because_en_US, - name.c_str(), num); + name, num); return {num, std::move(reason)}; } return {}; @@ -857,9 +864,8 @@ std::pair<WithReason<int64_t>, bool> GetAffectedNestDepthWithReason( std::string name{parser::omp::GetUpperName( llvm::omp::Clause::OMPC_permutation, version)}; Reason reason; - reason.Say(spec.source, - "%s clause was not specified, %s(2, 1) was assumed"_because_en_US, - name.c_str(), name.c_str()); + reason.Say( + spec.source, MsgClauseAbsentAssume, name, "a permutation (2, 1)"); return {{2, std::move(reason)}, true}; } case llvm::omp::Directive::OMPD_stripe: @@ -879,9 +885,7 @@ std::pair<WithReason<int64_t>, bool> GetAffectedNestDepthWithReason( std::string name{ parser::omp::GetUpperName(llvm::omp::Clause::OMPC_depth, version)}; Reason reason; - reason.Say(spec.source, - "%s clause was not specified, a value of 1 was assumed"_because_en_US, - name.c_str()); + reason.Say(spec.source, MsgClauseAbsentAssume, name, "a value of 1"); return {{1, std::move(reason)}, true}; } case llvm::omp::Directive::OMPD_reverse: @@ -919,15 +923,14 @@ WithReason<std::pair<int64_t, int64_t>> GetAffectedLoopRangeWithReason( reason.Say(clause->source, "%s clause was specified with a count of %" PRId64 " starting at loop %" PRId64 ""_because_en_US, - name.c_str(), *count, *first); + name, *count, *first); return {std::make_pair(*first, *count), std::move(reason)}; } // If LOOPRANGE was not found, return {1, -1}, where -1 means "the whole // associated sequence". Reason reason; - reason.Say(spec.source, - "%s clause was not specified, the entire sequence is affected by"_because_en_US, - name.c_str()); + reason.Say( + spec.source, MsgClauseAbsentAssume, name, "the entire loop sequence"); return {std::make_pair(1, -1), std::move(reason)}; } @@ -1096,8 +1099,8 @@ WithReason<int64_t> LoopSequence::calculateLength() const { llvm::omp::Directive dir{beginSpec.DirId()}; if (!IsLoopTransforming(dir)) { Reason reason; - reason.Say(beginSpec.DirName().source, - "This construct does not result in a loop nest or a loop sequence"_because_en_US); + reason.Say(beginSpec.DirName().source, MsgConstructDoesNotResult, + GetUpperName(dir, version_), "a loop nest or a loop sequence"); return {0, std::move(reason)}; } @@ -1126,9 +1129,9 @@ WithReason<int64_t> LoopSequence::calculateLength() const { parser::omp::FindClause(beginSpec, llvm::omp::Clause::OMPC_looprange)}; if (!clause) { Reason reason; - reason.Say(beginSpec.DirName().source, - "%s clause was not specified, all loops in the sequence are fused"_because_en_US, - GetUpperName(llvm::omp::Clause::OMPC_looprange, version_)); + reason.Say(beginSpec.DirName().source, MsgClauseAbsentAssume, + GetUpperName(llvm::omp::Clause::OMPC_looprange, version_), + "the entire loop sequence"); return {1, std::move(reason)}; } @@ -1141,7 +1144,7 @@ WithReason<int64_t> LoopSequence::calculateLength() const { int64_t result{1 + *nestedLength.value - *count}; Reason reason; reason.Say(beginSpec.DirName().source, - "Out of %" PRId64 " loops, %" PRId64 " were fused"_because_en_US, + "Out of %" PRId64 " loops, %" PRId64 " are fused"_because_en_US, *nestedLength.value, *count); return {result, std::move(reason)}; } @@ -1168,6 +1171,25 @@ WithReason<int64_t> LoopSequence::getNestedLength() const { return sum; } +static void ResetIfPositiveWithReason( + WithReason<int64_t> &quantity, const Reason &reason) { + if (quantity.value > 0) { + quantity.value = 0; + quantity.reason.Append(reason); + } +} + +static void ResetIfPositiveWithReason(WithReason<int64_t> &quantity, + parser::CharBlock source, parser::MessageFixedText msg) { + if (quantity.value > 0) { + quantity.value = 0; + quantity.reason.Say(source, msg); + } +} + +static Reason whyNotWellFormed( + const parser::ExecutionPartConstruct &badCode, bool isSequence); + LoopSequence::Depth LoopSequence::calculateDepths() const { // Get the length of the nested sequence. The invalidIC_ and opaqueIC_ // members do not count canonical loop nests, but there can only be one @@ -1177,38 +1199,22 @@ LoopSequence::Depth LoopSequence::calculateDepths() const { // entry_), and use it as the basis for the depths of entry_->owner. auto [semaDepth, perfDepth]{getNestedDepths()}; if (invalidIC_) { - parser::CharBlock source{*parser::GetSource(*invalidIC_)}; - if (semaDepth.value > 9) { - semaDepth.value = 0; - semaDepth.reason.Say( - source, "This is not a valid intervening code"_because_en_US); - } - if (perfDepth.value > 0) { - perfDepth.value = 0; - perfDepth.reason.Say( - source, "This is not a valid intervening code"_because_en_US); - } + auto whyNot{whyNotWellFormed(*invalidIC_, false)}; + ResetIfPositiveWithReason(semaDepth, whyNot); + ResetIfPositiveWithReason(perfDepth, whyNot); } else if (opaqueIC_) { + auto message{"This code prevents perfect nesting"_because_en_US}; parser::CharBlock source{*parser::GetSource(*opaqueIC_)}; - if (perfDepth.value > 0) { - perfDepth.value = 0; - perfDepth.reason.Say( - source, "This code prevents perfect nesting"_because_en_US); - } + ResetIfPositiveWithReason(perfDepth, source, message); } if (nestedLength.value.value_or(0) != 1) { // This may simply be the bottom of the loop nest. Only emit messages // if the depths are reset back to 0. if (entry_->owner) { + auto message{"This construct does not contain a loop nest"_because_en_US}; parser::CharBlock source{*parser::GetSource(*entry_->owner)}; - if (semaDepth.value > 0) { - semaDepth.reason.Say(source, - "This construct does not contain a loop nest"_because_en_US); - } - if (perfDepth.value > 9) { - perfDepth.reason.Say(source, - "This construct does not contain a loop nest"_because_en_US); - } + ResetIfPositiveWithReason(semaDepth, source, message); + ResetIfPositiveWithReason(perfDepth, source, message); } semaDepth.value = perfDepth.value = 0; } @@ -1249,10 +1255,12 @@ LoopSequence::Depth LoopSequence::calculateDepths() const { if (*required == -1 || *required == *nestedLength.value) { return Depth{value, value}; } + std::string name{ + GetUpperName(llvm::omp::Directive::OMPD_fuse, version_)}; Reason reason(std::move(range.reason)); - reason.Say(beginSpec.DirName().source, - "%s construct results in a proper loop-sequence"_because_en_US, - GetUpperName(llvm::omp::Directive::OMPD_fuse, version_)); + reason.Say(beginSpec.DirName().source, MsgConstructDoesNotResult, + "This " + name + " construct", + "a loop nest, but a proper loop sequence"); return Depth{{1, reason}, {1, reason}}; } } @@ -1281,8 +1289,8 @@ LoopSequence::Depth LoopSequence::calculateDepths() const { case llvm::omp::Directive::OMPD_unroll: if (isFullUnroll) { Reason reason; - reason.Say(beginSpec.DirName().source, - "Fully unrolled loop does not result in a loop nest"_because_en_US); + reason.Say(beginSpec.DirName().source, MsgConstructDoesNotResult, + "Fully unrolled loop", "a loop nest"); return Depth{{0, reason}, {0, reason}}; } // If this is not a full unroll then look for a PARTIAL clause. @@ -1326,4 +1334,64 @@ LoopSequence::Depth LoopSequence::getNestedDepths() const { } return children_.front().depth_; } + +static bool IsDoConcurrent(const parser::ExecutionPartConstruct &x) { + if (auto *loop{parser::Unwrap<parser::DoConstruct>(x)}) { + return loop->IsDoConcurrent(); + } + return false; +} + +static Reason whyNotWellFormed( + const parser::ExecutionPartConstruct &badCode, bool isSequence) { + Reason reason; + parser::CharBlock source{*parser::GetSource(badCode)}; + if (auto *omp{parser::Unwrap<parser::OpenMPLoopConstruct>(badCode)}) { + if (IsFullUnroll(*omp)) { + reason.Say(source, MsgConstructDoesNotResult, "Fully unrolled loop", + isSequence ? "a loop nest or a loop sequence" : "a loop nest"); + } else if (!IsLoopTransforming(omp->BeginDir().DirId())) { + reason.Say(source, + "Only loop-transforming constructs are allowed inside loop constructs"_because_en_US); + } + return reason; + } + + if (auto *loop{parser::Unwrap<parser::DoConstruct>(badCode)}) { + if (loop->IsDoWhile()) { + reason.Say(source, MsgNotValidAffectedLoop, "DO WHILE loop"); + } else if (loop->IsDoConcurrent()) { + reason.Say(source, MsgNotValidAffectedLoop, "DO CONCURRENT loop"); + } else if (!loop->GetLoopControl()) { + reason.Say( + source, MsgNotValidAffectedLoop, "DO loop without loop control"); + } + if (reason) { + return reason; + } + } + reason.Say(source, + "The %s contains code that prevents it from being canonical at this nesting level"_because_en_US, + isSequence ? "sequence" : "nest"); + return reason; +} + +WithReason<bool> LoopSequence::isWellFormedSequence() const { + const parser::ExecutionPartConstruct *badCode{ + invalidIC_ ? invalidIC_ : opaqueIC_}; + if (badCode) { + return {false, whyNotWellFormed(*badCode, true)}; + } + return {true, Reason()}; +} + +WithReason<bool> LoopSequence::isWellFormedNest() const { + // DO CONCURRENT is allowed at the top level in OpenMP 6.0+. + if (invalidIC_) { + if (version_ < 60 || !IsDoConcurrent(*invalidIC_)) { + return {false, whyNotWellFormed(*invalidIC_, false)}; + } + } + return {true, Reason()}; +} } // namespace Fortran::semantics::omp diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 7fca738d43ca6..3d55568c0cd03 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -2395,11 +2395,6 @@ void OmpAttributeVisitor::PrivatizeAssociatedLoopIndexAndCheckLoopLevel( context_.Say(clause->source, "DO CONCURRENT loops cannot be used with the COLLAPSE clause."_err_en_US); } - } else { - auto &stmt = - std::get<parser::Statement<parser::NonLabelDoStmt>>(loop->t); - context_.Say(stmt.source, - "DO CONCURRENT loops cannot form part of a loop nest."_err_en_US); } } // go through all the nested do-loops and resolve index variables diff --git a/flang/test/Parser/OpenMP/interchange-fail.f90 b/flang/test/Parser/OpenMP/interchange-fail.f90 deleted file mode 100644 index d83ef1746f30f..0000000000000 --- a/flang/test/Parser/OpenMP/interchange-fail.f90 +++ /dev/null @@ -1,31 +0,0 @@ -! RUN: split-file %s %t -! RUN: not %flang_fc1 -fsyntax-only -fopenmp -fopenmp-version=60 %t/stray_end1.f90 2>&1 | Fi... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/188025 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
