From 3a141d6729e26a7eab821b86eee240ab4bfa322f Mon Sep 17 00:00:00 2001 From: Michael Kruse Date: Tue, 23 Sep 2025 12:59:14 +0200 Subject: [PATCH 1/4] Add perfect-nest and rectangular loop nest tests --- flang/lib/Semantics/resolve-directives.cpp | 146 ++++++++++++++++++++- flang/test/Semantics/OpenMP/do08.f90 | 1 + flang/test/Semantics/OpenMP/do13.f90 | 1 + flang/test/Semantics/OpenMP/do22.f90 | 73 +++++++++++ 4 files changed, 215 insertions(+), 6 deletions(-) create mode 100644 flang/test/Semantics/OpenMP/do22.f90 diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 2d1bec9968593..5f2c9f676099c 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -149,6 +149,9 @@ template class DirectiveAttributeVisitor { dataSharingAttributeObjects_.clear(); } bool HasDataSharingAttributeObject(const Symbol &); + std::tuple + GetLoopBounds(const parser::DoConstruct &); const parser::Name *GetLoopIndex(const parser::DoConstruct &); const parser::DoConstruct *GetDoConstructIf( const parser::ExecutionPartConstruct &); @@ -933,6 +936,13 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor { privateDataSharingAttributeObjects_.clear(); } + /// Check that loops in the loop nest are perfectly nested, as well that lower + /// bound, upper bound, and step expressions do not use the iv + /// of a surrounding loop of the associated loops nest. + /// We do not support non-perfectly nested loops not non-rectangular loops yet + /// (both introduced in OpenMP 5.0) + void CheckPerfectNestAndRectangularLoop(const parser::OpenMPLoopConstruct &x); + // Predetermined DSA rules void PrivatizeAssociatedLoopIndexAndCheckLoopLevel( const parser::OpenMPLoopConstruct &); @@ -1009,14 +1019,15 @@ bool DirectiveAttributeVisitor::HasDataSharingAttributeObject( } template -const parser::Name *DirectiveAttributeVisitor::GetLoopIndex( - const parser::DoConstruct &x) { +std::tuple +DirectiveAttributeVisitor::GetLoopBounds(const parser::DoConstruct &x) { using Bounds = parser::LoopControl::Bounds; if (x.GetLoopControl()) { if (const Bounds * b{std::get_if(&x.GetLoopControl()->u)}) { - return &b->name.thing; - } else { - return nullptr; + auto &&step = b->step; + return {&b->name.thing, &b->lower, &b->upper, + step.has_value() ? &step.value() : nullptr}; } } else { context_ @@ -1024,8 +1035,15 @@ const parser::Name *DirectiveAttributeVisitor::GetLoopIndex( "Loop control is not present in the DO LOOP"_err_en_US) .Attach(GetContext().directiveSource, "associated with the enclosing LOOP construct"_en_US); - return nullptr; } + return {nullptr, nullptr, nullptr, nullptr}; +} + +template +const parser::Name *DirectiveAttributeVisitor::GetLoopIndex( + const parser::DoConstruct &x) { + auto &&[iv, lb, ub, step] = GetLoopBounds(x); + return iv; } template @@ -1957,6 +1975,7 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) { } } } + CheckPerfectNestAndRectangularLoop(x); PrivatizeAssociatedLoopIndexAndCheckLoopLevel(x); ordCollapseLevel = GetNumAffectedLoopsFromLoopConstruct(x) + 1; return true; @@ -2152,6 +2171,121 @@ void OmpAttributeVisitor::CollectNumAffectedLoopsFromClauses( } } +void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( + const parser::OpenMPLoopConstruct + &x) { // GetAssociatedLoopLevelFromClauses(clauseList); + auto &&dirContext = GetContext(); + std::int64_t dirDepth{dirContext.associatedLoopLevel}; + if (dirDepth <= 0) + return; + + Symbol::Flag ivDSA; + if (!llvm::omp::allSimdSet.test(GetContext().directive)) { + ivDSA = Symbol::Flag::OmpPrivate; + } else if (dirDepth == 1) { + ivDSA = Symbol::Flag::OmpLinear; + } else { + ivDSA = Symbol::Flag::OmpLastPrivate; + } + + auto checkExprHasSymbols = [&](llvm::SmallVector &ivs, + const parser::ScalarExpr *bound) { + if (ivs.empty()) + return; + + if (auto boundExpr{semantics::AnalyzeExpr(context_, *bound)}) { + semantics::UnorderedSymbolSet boundSyms = + evaluate::CollectSymbols(*boundExpr); + for (auto iv : ivs) { + if (boundSyms.count(*iv) != 0) { + // TODO: Point to occurence of iv in boundExpr, directiveSource as a + // note + context_.Say(dirContext.directiveSource, + "Trip count must be computable and invariant"_err_en_US); + } + } + } + }; + + // Skip over loop transformation directives + const parser::OpenMPLoopConstruct *innerMostLoop = &x; + const parser::NestedConstruct *innerMostNest = nullptr; + while (auto &optLoopCons{ + std::get>(innerMostLoop->t)}) { + innerMostNest = &(optLoopCons.value()); + if (const auto *innerLoop{ + std::get_if>( + innerMostNest)}) { + innerMostLoop = &(innerLoop->value()); + } else + break; + } + + if (!innerMostNest) + return; + const auto &outer{std::get_if(innerMostNest)}; + if (!outer) + return; + + llvm::SmallVector ivs; + int curLevel = 0; + const parser::DoConstruct *loop{outer}; + while (true) { + auto [iv, lb, ub, step] = GetLoopBounds(*loop); + + if (lb) + checkExprHasSymbols(ivs, lb); + if (ub) + checkExprHasSymbols(ivs, ub); + if (step) + checkExprHasSymbols(ivs, step); + if (iv) { + if (auto *symbol{ResolveOmp(*iv, ivDSA, currScope())}) + ivs.push_back(symbol); + } + + // Stop after processing all affected loops + if (curLevel + 1 >= dirDepth) + break; + + // Recurse into nested loop + const auto &block{std::get(loop->t)}; + if (block.empty()) { + // Insufficient number of nested loops already reported by + // CheckAssocLoopLevel() + break; + } + + loop = GetDoConstructIf(block.front()); + if (!loop) { + // Insufficient number of nested loops already reported by + // CheckAssocLoopLevel() + break; + } + + auto checkPerfectNest = [&, this]() { + auto blockSize = block.size(); + if (blockSize <= 1) + return; + + if (parser::Unwrap(x)) + blockSize -= 1; + + if (blockSize <= 1) + return; + + // Non-perfectly nested loop + // TODO: Point to non-DO statement, directiveSource as a note + context_.Say(dirContext.directiveSource, + "Canonical loop nest must be perfectly nested."_err_en_US); + }; + + checkPerfectNest(); + + ++curLevel; + } +} + // 2.15.1.1 Data-sharing Attribute Rules - Predetermined // - The loop iteration variable(s) in the associated do-loop(s) of a do, // parallel do, taskloop, or distribute construct is (are) private. diff --git a/flang/test/Semantics/OpenMP/do08.f90 b/flang/test/Semantics/OpenMP/do08.f90 index 5143dff0dd315..bb3c1d0cd3855 100644 --- a/flang/test/Semantics/OpenMP/do08.f90 +++ b/flang/test/Semantics/OpenMP/do08.f90 @@ -61,6 +61,7 @@ program omp !$omp end do + !ERROR: Canonical loop nest must be perfectly nested. !ERROR: The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct. !$omp do collapse(3) do 60 i=2,200,2 diff --git a/flang/test/Semantics/OpenMP/do13.f90 b/flang/test/Semantics/OpenMP/do13.f90 index 6e9d1dddade4c..8f7844f4136f9 100644 --- a/flang/test/Semantics/OpenMP/do13.f90 +++ b/flang/test/Semantics/OpenMP/do13.f90 @@ -59,6 +59,7 @@ program omp !$omp end do + !ERROR: Canonical loop nest must be perfectly nested. !ERROR: The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct. !$omp do collapse(3) do 60 i=1,10 diff --git a/flang/test/Semantics/OpenMP/do22.f90 b/flang/test/Semantics/OpenMP/do22.f90 new file mode 100644 index 0000000000000..9d96d3af54e5c --- /dev/null +++ b/flang/test/Semantics/OpenMP/do22.f90 @@ -0,0 +1,73 @@ +! RUN: %python %S/../test_errors.py %s %flang -fopenmp +! Check for existence of loop following a DO directive + +subroutine do_imperfectly_nested_before + integer i, j + + !ERROR: The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct. + !$omp do collapse(2) + do i = 1, 10 + print *, i + do j = 1, 10 + print *, i, j + end do + end do + !$omp end do +end subroutine + + +subroutine do_imperfectly_nested_behind + integer i, j + + !ERROR: Canonical loop nest must be perfectly nested. + !$omp do collapse(2) + do i = 1, 10 + do j = 1, 10 + print *, i, j + end do + print *, i + end do + !$omp end do +end subroutine + + +subroutine do_nonrectangular_lb + integer i, j + + !ERROR: Trip count must be computable and invariant + !$omp do collapse(2) + do i = 1, 10 + do j = i, 10 + print *, i, j + end do + end do + !$omp end do +end subroutine + + +subroutine do_nonrectangular_ub + integer i, j + + !ERROR: Trip count must be computable and invariant + !$omp do collapse(2) + do i = 1, 10 + do j = 0, i + print *, i, j + end do + end do + !$omp end do +end subroutine + + +subroutine do_nonrectangular_step + integer i, j + + !ERROR: Trip count must be computable and invariant + !$omp do collapse(2) + do i = 1, 10 + do j = 1, 10, i + print *, i, j + end do + end do + !$omp end do +end subroutine From 493442453cbac2366aa89abde361f7badaad4948 Mon Sep 17 00:00:00 2001 From: Michael Kruse Date: Tue, 23 Sep 2025 16:39:44 +0200 Subject: [PATCH 2/4] Fix symbol resolution --- flang/lib/Semantics/resolve-directives.cpp | 48 ++++++++++------------ 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 5f2c9f676099c..a0109324e546c 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -1975,7 +1975,10 @@ bool OmpAttributeVisitor::Pre(const parser::OpenMPLoopConstruct &x) { } } } + + // Must be done before iv privatization CheckPerfectNestAndRectangularLoop(x); + PrivatizeAssociatedLoopIndexAndCheckLoopLevel(x); ordCollapseLevel = GetNumAffectedLoopsFromLoopConstruct(x) + 1; return true; @@ -2172,37 +2175,29 @@ void OmpAttributeVisitor::CollectNumAffectedLoopsFromClauses( } void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( - const parser::OpenMPLoopConstruct - &x) { // GetAssociatedLoopLevelFromClauses(clauseList); - auto &&dirContext = GetContext(); + const parser::OpenMPLoopConstruct &x) { + auto &dirContext = GetContext(); std::int64_t dirDepth{dirContext.associatedLoopLevel}; if (dirDepth <= 0) return; - Symbol::Flag ivDSA; - if (!llvm::omp::allSimdSet.test(GetContext().directive)) { - ivDSA = Symbol::Flag::OmpPrivate; - } else if (dirDepth == 1) { - ivDSA = Symbol::Flag::OmpLinear; - } else { - ivDSA = Symbol::Flag::OmpLastPrivate; - } - auto checkExprHasSymbols = [&](llvm::SmallVector &ivs, const parser::ScalarExpr *bound) { if (ivs.empty()) return; - - if (auto boundExpr{semantics::AnalyzeExpr(context_, *bound)}) { - semantics::UnorderedSymbolSet boundSyms = - evaluate::CollectSymbols(*boundExpr); - for (auto iv : ivs) { - if (boundSyms.count(*iv) != 0) { - // TODO: Point to occurence of iv in boundExpr, directiveSource as a - // note - context_.Say(dirContext.directiveSource, - "Trip count must be computable and invariant"_err_en_US); - } + auto boundExpr{semantics::AnalyzeExpr(context_, *bound)}; + if (!boundExpr) + return; + semantics::UnorderedSymbolSet boundSyms = + evaluate::CollectSymbols(*boundExpr); + if (boundSyms.empty()) + return; + for (Symbol *iv : ivs) { + if (boundSyms.count(*iv) != 0) { + // TODO: Point to occurence of iv in boundExpr, directiveSource as a + // note + context_.Say(dirContext.directiveSource, + "Trip count must be computable and invariant"_err_en_US); } } }; @@ -2217,8 +2212,9 @@ void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( std::get_if>( innerMostNest)}) { innerMostLoop = &(innerLoop->value()); - } else + } else { break; + } } if (!innerMostNest) @@ -2228,7 +2224,7 @@ void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( return; llvm::SmallVector ivs; - int curLevel = 0; + int curLevel{0}; const parser::DoConstruct *loop{outer}; while (true) { auto [iv, lb, ub, step] = GetLoopBounds(*loop); @@ -2240,7 +2236,7 @@ void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( if (step) checkExprHasSymbols(ivs, step); if (iv) { - if (auto *symbol{ResolveOmp(*iv, ivDSA, currScope())}) + if (auto *symbol{currScope().FindSymbol(iv->source)}) ivs.push_back(symbol); } From f1f38a6d69910b6beb48e6202bef2c52181459ad Mon Sep 17 00:00:00 2001 From: Michael Kruse Date: Wed, 24 Sep 2025 13:53:49 +0200 Subject: [PATCH 3/4] Address review by @tblah --- flang/lib/Semantics/resolve-directives.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index a0109324e546c..3cead6030192e 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -149,10 +149,24 @@ template class DirectiveAttributeVisitor { dataSharingAttributeObjects_.clear(); } bool HasDataSharingAttributeObject(const Symbol &); + + /// Extract the iv and bounds of a DO loop: + /// 1. The loop index/induction variable + /// 2. The lower bound + /// 3. The upper bound + /// 4. The step/increment (or nullptr if not present) + /// + /// Each returned tuple value can be nullptr if not present. Diagnoses an + /// error if the the DO loop is a DO WHILE or DO CONCURRENT loop. std::tuple GetLoopBounds(const parser::DoConstruct &); + + /// Extract the loop index/induction variable from a DO loop. Diagnoses an + /// error if the the DO loop is a DO WHILE or DO CONCURRENT loop and returns + /// nullptr. const parser::Name *GetLoopIndex(const parser::DoConstruct &); + const parser::DoConstruct *GetDoConstructIf( const parser::ExecutionPartConstruct &); Symbol *DeclareNewAccessEntity(const Symbol &, Symbol::Flag, Scope &); @@ -1025,7 +1039,7 @@ DirectiveAttributeVisitor::GetLoopBounds(const parser::DoConstruct &x) { using Bounds = parser::LoopControl::Bounds; if (x.GetLoopControl()) { if (const Bounds * b{std::get_if(&x.GetLoopControl()->u)}) { - auto &&step = b->step; + auto &step = b->step; return {&b->name.thing, &b->lower, &b->upper, step.has_value() ? &step.value() : nullptr}; } @@ -1042,8 +1056,7 @@ DirectiveAttributeVisitor::GetLoopBounds(const parser::DoConstruct &x) { template const parser::Name *DirectiveAttributeVisitor::GetLoopIndex( const parser::DoConstruct &x) { - auto &&[iv, lb, ub, step] = GetLoopBounds(x); - return iv; + return std::get(GetLoopBounds(x)); } template @@ -2176,7 +2189,7 @@ void OmpAttributeVisitor::CollectNumAffectedLoopsFromClauses( void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( const parser::OpenMPLoopConstruct &x) { - auto &dirContext = GetContext(); + auto &dirContext{GetContext()}; std::int64_t dirDepth{dirContext.associatedLoopLevel}; if (dirDepth <= 0) return; From d7ea10a6c8062ba486bff3353daf05833d614c8e Mon Sep 17 00:00:00 2001 From: Michael Kruse Date: Thu, 25 Sep 2025 12:08:33 +0200 Subject: [PATCH 4/4] Address review by @tblah --- flang/lib/Semantics/resolve-directives.cpp | 28 ++++++++++++---------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp index 3cead6030192e..be4b7f77b85c7 100644 --- a/flang/lib/Semantics/resolve-directives.cpp +++ b/flang/lib/Semantics/resolve-directives.cpp @@ -2201,8 +2201,8 @@ void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( auto boundExpr{semantics::AnalyzeExpr(context_, *bound)}; if (!boundExpr) return; - semantics::UnorderedSymbolSet boundSyms = - evaluate::CollectSymbols(*boundExpr); + semantics::UnorderedSymbolSet boundSyms{ + evaluate::CollectSymbols(*boundExpr)}; if (boundSyms.empty()) return; for (Symbol *iv : ivs) { @@ -2215,24 +2215,26 @@ void OmpAttributeVisitor::CheckPerfectNestAndRectangularLoop( } }; - // Skip over loop transformation directives - const parser::OpenMPLoopConstruct *innerMostLoop = &x; - const parser::NestedConstruct *innerMostNest = nullptr; - while (auto &optLoopCons{ - std::get>(innerMostLoop->t)}) { - innerMostNest = &(optLoopCons.value()); - if (const auto *innerLoop{ + // Find the associated region by skipping nested loop-associated constructs + // such as loop transformations + const parser::NestedConstruct *innermostAssocRegion{nullptr}; + const parser::OpenMPLoopConstruct *innermostConstruct{&x}; + while (const auto &innerAssocStmt{ + std::get>( + innermostConstruct->t)}) { + innermostAssocRegion = &(innerAssocStmt.value()); + if (const auto *innerConstruct{ std::get_if>( - innerMostNest)}) { - innerMostLoop = &(innerLoop->value()); + innermostAssocRegion)}) { + innermostConstruct = &innerConstruct->value(); } else { break; } } - if (!innerMostNest) + if (!innermostAssocRegion) return; - const auto &outer{std::get_if(innerMostNest)}; + const auto &outer{std::get_if(innermostAssocRegion)}; if (!outer) return;