[flang-commits] [flang] 6ac7a9f - [flang][PFT-to-MLIR] Do not wrap the DO of an OpenMP loop construct (#216280)
via flang-commits
flang-commits at lists.llvm.org
Wed Aug 19 22:12:45 PDT 2026
Author: Kareem Ergawy
Date: 2026-08-20T07:12:40+02:00
New Revision: 6ac7a9f96452117c3498d7030398101103f6c3d3
URL: https://github.com/llvm/llvm-project/commit/6ac7a9f96452117c3498d7030398101103f6c3d3
DIFF: https://github.com/llvm/llvm-project/commit/6ac7a9f96452117c3498d7030398101103f6c3d3.diff
LOG: [flang][PFT-to-MLIR] Do not wrap the DO of an OpenMP loop construct (#216280)
A DO attached to an OpenMP loop directive is driven directly into the
loop op by OpenMPLoopConstruct lowering, so folding it into an
scf.execute_region hides the iteration from that op and crashes lowering
when the loop body is unstructured.
Extend the wrappability check with isOmpLoopBody(), the OpenMP analogue
of the existing isAccLoopBody(): the immediate body DO, or one of the N
associated iterator DOs under collapse(N) / ordered(N), is not
wrappable. The shared parts of both checks -- collecting the enclosing
DO chain, evaluating a clause's loop count, and the depth test -- are
factored into helpers.
Co-authored-by: Claude
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply at anthropic.com>
Added:
flang/test/Lower/OpenMP/wsloop-unstructured-cycle.f90
Modified:
flang/include/flang/Lower/PFTBuilder.h
flang/lib/Lower/Bridge.cpp
flang/lib/Lower/PFTBuilder.cpp
Removed:
################################################################################
diff --git a/flang/include/flang/Lower/PFTBuilder.h b/flang/include/flang/Lower/PFTBuilder.h
index 490b2a46c18bf..8a59dd6638b71 100644
--- a/flang/include/flang/Lower/PFTBuilder.h
+++ b/flang/include/flang/Lower/PFTBuilder.h
@@ -882,8 +882,10 @@ void visitAllSymbols(const Evaluation &eval,
std::function<void(const semantics::Symbol &)> callBack);
/// Return true when \p eval is an unstructured DO or IF construct that can
-/// folded into a self-contained scf.execute_region.
-bool isWrappableConstruct(const Evaluation &eval);
+/// folded into a self-contained scf.execute_region. \p semaCtx is needed to
+/// determine how many loops a directive applies to.
+bool isWrappableConstruct(const Evaluation &eval,
+ const semantics::SemanticsContext &semaCtx);
} // namespace Fortran::lower::pft
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index c52b8bd67a111..e2db2d20a4b4f 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -2568,7 +2568,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
mlir::scf::ExecuteRegionOp
wrapUnstructuredConstruct(Fortran::lower::pft::Evaluation &eval,
mlir::Block *&savedExitBlock) {
- if (!Fortran::lower::pft::isWrappableConstruct(eval))
+ if (!Fortran::lower::pft::isWrappableConstruct(
+ eval, bridge.getSemanticsContext()))
return nullptr;
mlir::Location loc = toLocation();
@@ -3177,7 +3178,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
// allocated for us — startNewFunction runs resetEvaluationBlocks before
// each entry-point pass, so we can trust the pointer isn't stale from a
// previous pass.
- if (Fortran::lower::pft::isWrappableConstruct(eval) &&
+ if (Fortran::lower::pft::isWrappableConstruct(
+ eval, bridge.getSemanticsContext()) &&
eval.hasNestedEvaluations()) {
Fortran::lower::pft::Evaluation &firstStmt =
eval.getFirstNestedEvaluation();
@@ -6310,7 +6312,8 @@ class FirConverter : public Fortran::lower::AbstractConverter {
if (eval.isNewBlock)
eval.block = builder->createBlock(region);
if (eval.isConstruct() || eval.isDirective()) {
- if (Fortran::lower::pft::isWrappableConstruct(eval)) {
+ if (Fortran::lower::pft::isWrappableConstruct(
+ eval, bridge.getSemanticsContext())) {
// The wrap owns internal blocks; only create the entry block here
// so the enclosing CFG can branch to it.
if (eval.hasNestedEvaluations()) {
diff --git a/flang/lib/Lower/PFTBuilder.cpp b/flang/lib/Lower/PFTBuilder.cpp
index e5c91ac101679..c4a16fcc12285 100644
--- a/flang/lib/Lower/PFTBuilder.cpp
+++ b/flang/lib/Lower/PFTBuilder.cpp
@@ -11,12 +11,14 @@
#include "flang/Lower/Support/Utils.h"
#include "flang/Parser/dump-parse-tree.h"
#include "flang/Parser/parse-tree-visitor.h"
+#include "flang/Semantics/openmp-utils.h"
#include "flang/Semantics/semantics.h"
#include "flang/Semantics/tools.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/IntervalMap.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
+#include <algorithm>
#include <limits>
#define DEBUG_TYPE "flang-pft"
@@ -1247,7 +1249,7 @@ class PFTBuilder {
// wrap pass will fold this construct into a self-contained
// scf.execute_region, in which case the parent sees only a single op.
if (parentConstruct && eval.isUnstructured &&
- !lower::pft::isWrappableConstruct(eval))
+ !lower::pft::isWrappableConstruct(eval, semanticsContext))
parentConstruct->isUnstructured = true;
// The successor of a branch starts a new block.
@@ -2497,78 +2499,137 @@ hasIncomingBranch(const Fortran::lower::pft::Evaluation &construct) {
return walk(funit->evaluationList);
}
-/// True if \p eval is a DoConstruct driven directly into an enclosing acc.loop
-/// by the OpenACCLoopConstruct / OpenACCCombinedConstruct lowering — the
-/// immediate body DO, or one of the N collapsed iterator DOs reached by
-/// walking down from the body DO under a `collapse(N)` clause.
-static bool isAccLoopBody(const Fortran::lower::pft::Evaluation &eval) {
+/// The DoConstructs enclosing (and including) a DO evaluation; index k holds
+/// the one at depth k, so index 0 is the evaluation's own DO.
+using DoConstructChain = llvm::SmallVector<const parser::DoConstruct *, 4>;
+
+/// Value of \p intExpr, or INT64_MAX if it isn't a compile-time constant.
+///
+/// This is used for the loop count of the OpenACC `collapse(N)` clause, which
+/// requires N to be a positive scalar integer constant expression. A constant
+/// too large to be represented in an int64_t would be truncated by ToInt64
+/// rather than reported, so a value that is not positive is treated like a
+/// non-constant one rather than being used. INT64_MAX makes every enclosing
+/// loop count as associated with the directive, which is the safe answer here
+/// because it only prevents wrapping.
+///
+/// OpenMP uses semantics::omp::GetAffectedNestDepthWithReason instead, which
+/// covers the loop transforming directives as well as the clauses.
+static int64_t
+constantValueOrMax(const parser::ScalarIntConstantExpr &intExpr) {
+ if (const auto *expr = semantics::GetExpr(intExpr))
+ if (auto v = evaluate::ToInt64(*expr))
+ if (*v > 0)
+ return *v;
+ return std::numeric_limits<int64_t>::max();
+}
+
+/// Fill \p chain with the DoConstruct at each depth above (and including)
+/// \p eval, and return the innermost enclosing evaluation that is not a
+/// DoConstruct — the one a directive would be attached to. Returns null if
+/// \p eval is not a DoConstruct or has no such enclosing evaluation.
+static const Fortran::lower::pft::Evaluation *
+collectEnclosingDoChain(const Fortran::lower::pft::Evaluation &eval,
+ DoConstructChain &chain) {
const auto *doConstruct = eval.getIf<parser::DoConstruct>();
if (!doConstruct)
- return false;
- // N from `collapse(N)`, or 1 if no clause. eval at depth d from the body
- // (d == 0 means eval IS the body) is a collapsed iterator iff d < N. If the
- // Collapse value isn't a compile-time constant, be conservative and treat
- // every DO in the chain as collapsed (INT64_MAX) — wrapping is opt-in and
- // a false "is collapsed" is safer than a false "is not".
- auto collapseN = [](const parser::AccClauseList &cl) -> int64_t {
- for (const parser::AccClause &c : cl.v)
- if (const auto *cc = std::get_if<parser::AccClause::Collapse>(&c.u)) {
- const auto &intExpr = std::get<parser::ScalarIntConstantExpr>(cc->v.t);
- if (const auto *expr = semantics::GetExpr(intExpr))
- if (auto v = evaluate::ToInt64(*expr))
- return *v;
- return std::numeric_limits<int64_t>::max();
- }
- return 1;
- };
-
- // candidates[k] is the DoConstruct at depth k above (and including) eval.
- llvm::SmallVector<const parser::DoConstruct *, 4> candidates{doConstruct};
+ return nullptr;
+ chain.push_back(doConstruct);
for (const Fortran::lower::pft::Evaluation *p = eval.parentConstruct; p;
p = p->parentConstruct) {
if (const auto *d = p->getIf<parser::DoConstruct>()) {
- candidates.push_back(d);
+ chain.push_back(d);
continue;
}
+ return p;
+ }
+ return nullptr;
+}
- if (const auto *acc = p->getIf<parser::OpenACCConstruct>()) {
- const parser::DoConstruct *body = nullptr;
- int64_t n = 1;
- if (const auto *loop =
- std::get_if<parser::OpenACCLoopConstruct>(&acc->u)) {
- if (const auto &b =
- std::get<std::optional<parser::DoConstruct>>(loop->t))
- body = &b.value();
- n = collapseN(std::get<parser::AccClauseList>(std::get<0>(loop->t).t));
- } else if (const auto *comb =
- std::get_if<parser::OpenACCCombinedConstruct>(&acc->u)) {
- if (const auto &b =
- std::get<std::optional<parser::DoConstruct>>(comb->t))
- body = &b.value();
- n = collapseN(std::get<parser::AccClauseList>(std::get<0>(comb->t).t));
- }
+/// True if the DO at depth 0 of \p chain is one of the \p n loops a directive
+/// associates with itself, given that the directive's body DO is \p body.
+///
+/// \p body is the outermost candidate, so the evaluation sits at depth
+/// `index of body in chain` below it and is associated iff that depth < \p n.
+/// A \p body outside \p chain is not an error: OpenMPLoopConstruct's body is
+/// found by searching the construct's block (looking through a BLOCK
+/// construct), so it can name a DO that is not on this ancestor chain.
+static bool isAssociatedLoop(const DoConstructChain &chain,
+ const parser::DoConstruct *body, int64_t n) {
+ if (!body)
+ return false;
+ auto it = llvm::find(chain, body);
+ if (it == chain.end())
+ return false;
+ return std::distance(chain.begin(), it) < n;
+}
- if (body) {
- // body is at index `candidates.size()-1` (the outermost candidate);
- // eval at depth (candidates.size()-1) from body. Collapsed iff < N.
- auto it = llvm::find(candidates, body);
- if (it != candidates.end()) {
- int64_t depth = std::distance(candidates.begin(), it);
- if (depth < n)
- return true;
- }
- }
- }
+/// True if \p eval is a DoConstruct attached to an enclosing OpenACC loop.
+static bool isAccLoopBody(const Fortran::lower::pft::Evaluation &eval) {
+ DoConstructChain chain;
+ const Fortran::lower::pft::Evaluation *p =
+ collectEnclosingDoChain(eval, chain);
+ if (!p)
+ return false;
+
+ const auto *acc = p->getIf<parser::OpenACCConstruct>();
+ if (!acc)
+ return false;
- break;
+ // N from `collapse(N)`, or 1 if no clause.
+ auto collapseValue = [](const parser::AccClauseList &cl) -> int64_t {
+ for (const parser::AccClause &c : cl.v)
+ if (const auto *cc = std::get_if<parser::AccClause::Collapse>(&c.u))
+ return constantValueOrMax(
+ std::get<parser::ScalarIntConstantExpr>(cc->v.t));
+ return 1;
+ };
+
+ const parser::DoConstruct *body = nullptr;
+ int64_t n = 1;
+ if (const auto *loop = std::get_if<parser::OpenACCLoopConstruct>(&acc->u)) {
+ if (const auto &b = std::get<std::optional<parser::DoConstruct>>(loop->t))
+ body = &b.value();
+ n = collapseValue(std::get<parser::AccClauseList>(std::get<0>(loop->t).t));
+ } else if (const auto *comb =
+ std::get_if<parser::OpenACCCombinedConstruct>(&acc->u)) {
+ if (const auto &b = std::get<std::optional<parser::DoConstruct>>(comb->t))
+ body = &b.value();
+ n = collapseValue(std::get<parser::AccClauseList>(std::get<0>(comb->t).t));
}
- return false;
+ return isAssociatedLoop(chain, body, n);
+}
+
+/// True if \p eval is a DoConstruct attached to an enclosing OpenMP loop.
+static bool isOmpLoopBody(const Fortran::lower::pft::Evaluation &eval,
+ const semantics::SemanticsContext &semaCtx) {
+ DoConstructChain chain;
+ const Fortran::lower::pft::Evaluation *p =
+ collectEnclosingDoChain(eval, chain);
+ if (!p)
+ return false;
+
+ const auto *omp = p->getIf<parser::OpenMPConstruct>();
+ if (!omp)
+ return false;
+
+ const auto *loop = std::get_if<parser::OpenMPLoopConstruct>(&omp->u);
+ if (!loop)
+ return false;
+
+ unsigned version = semaCtx.langOptions().OpenMPVersion;
+ auto [depth, _] =
+ semantics::omp::GetAffectedNestDepthWithReason(loop->BeginDir(), version);
+ int64_t n = depth.value.value_or(1);
+
+ return isAssociatedLoop(chain, loop->GetNestedLoop(), n);
}
bool Fortran::lower::pft::isWrappableConstruct(
- const Fortran::lower::pft::Evaluation &eval) {
+ const Fortran::lower::pft::Evaluation &eval,
+ const Fortran::semantics::SemanticsContext &semaCtx) {
if (!wrapUnstructuredConstructsInExecuteRegion)
return false;
@@ -2581,10 +2642,10 @@ bool Fortran::lower::pft::isWrappableConstruct(
// Wrapping requires self-contained CFG.
//
- // Note: Loops attached to OpenACC constructs are not wrappable since
- // genOpenACCLoopFromDoConstruct takes over code-gen when a DoConstruct is
- // attached to an OpenACC directive. We might extend wrapping to such
- // unstructured loops later on if needed.
+ // Note: Loops attached to OpenACC/OpenMP constructs are not wrappable since
+ // the directive lowering (e.g. genOpenACCLoopFromDoConstruct) takes over
+ // code-gen when a DoConstruct is attached to such a directive. We might
+ // extend wrapping to such unstructured loops later on if needed.
return !hasUnwrappableInternals(eval) && !hasIncomingBranch(eval) &&
- !isAccLoopBody(eval);
+ !isAccLoopBody(eval) && !isOmpLoopBody(eval, semaCtx);
}
diff --git a/flang/test/Lower/OpenMP/wsloop-unstructured-cycle.f90 b/flang/test/Lower/OpenMP/wsloop-unstructured-cycle.f90
new file mode 100644
index 0000000000000..60fe63b8d29af
--- /dev/null
+++ b/flang/test/Lower/OpenMP/wsloop-unstructured-cycle.f90
@@ -0,0 +1,111 @@
+! RUN: bbc --wrap-unstructured-constructs-in-execute-region -emit-hlfir -fopenmp -o - %s | FileCheck %s --implicit-check-not=scf.execute_region
+
+! A DO associated with an OpenMP loop directive is lowered by the directive's
+! own code-gen. Such a DO must never be folded into an
+! scf.execute_region, even when wrapping is enabled and the loop is
+! unstructured -- here the IF-guarded CYCLE makes it so. The body's blocks
+! stay flat inside omp.loop_nest.
+!
+! --implicit-check-not on the RUN line asserts that no wrapping takes place
+! anywhere in the output.
+
+subroutine repro_final(x, y, n)
+ implicit none
+ integer n
+ double precision x(*), y(*)
+ integer i
+
+ !$omp do
+ do i = 1, n
+ if (x(i) > 0.0d0) then
+ y(1) = 0.0d0 ! any statement before CYCLE makes the loop unstructured
+ cycle
+ end if
+ y(2) = 1.0d0
+ end do
+ !$omp end do
+
+end subroutine repro_final
+
+! CHECK-LABEL: func.func @_QPrepro_final(
+! CHECK: omp.wsloop
+! CHECK: omp.loop_nest
+! CHECK: hlfir.assign
+! CHECK: cf.br ^bb[[TEST:[0-9]+]]
+! CHECK: ^bb[[TEST]]:
+! CHECK: arith.cmpf ogt
+! CHECK: cf.cond_br %{{[0-9]+}}, ^bb[[CYCLE:[0-9]+]], ^bb[[BODY:[0-9]+]]
+! CHECK: ^bb[[CYCLE]]:
+! CHECK: hlfir.assign
+! CHECK: cf.br ^bb[[EXIT:[0-9]+]]
+! CHECK: ^bb[[BODY]]:
+! CHECK: hlfir.assign
+! CHECK: cf.br ^bb[[EXIT]]
+! CHECK: ^bb[[EXIT]]:
+! CHECK: omp.yield
+
+! COLLAPSE(n) and ORDERED(n) both associate n loops with the directive, and
+! the loop transforming directives (TILE, INTERCHANGE, ...) associate as many
+! as their arguments describe. None of the associated loops may be wrapped.
+
+subroutine collapse_case(x, y, n)
+ implicit none
+ integer n
+ double precision x(*), y(*)
+ integer i, j
+
+ !$omp do collapse(2)
+ do i = 1, n
+ do j = 1, n
+ if (x(i) > 0.0d0) then
+ y(1) = 0.0d0
+ cycle
+ end if
+ y(2) = 1.0d0
+ end do
+ end do
+ !$omp end do
+
+end subroutine collapse_case
+
+! Both loops are associated with the directive, so the body stays flat inside
+! omp.loop_nest.
+! CHECK-LABEL: func.func @_QPcollapse_case(
+! CHECK: omp.wsloop
+! CHECK: omp.loop_nest ({{.*}}) {{.*}} collapse(2) {
+! CHECK: cf.cond_br
+! CHECK: omp.yield
+
+subroutine ordered_case(x, y, n)
+ implicit none
+ integer n
+ double precision x(*), y(*)
+ integer i, j
+
+ !$omp do ordered(2)
+ do i = 1, n
+ do j = 1, n
+ if (x(i) > 0.0d0) then
+ y(1) = 0.0d0
+ cycle
+ end if
+ y(2) = 1.0d0
+ end do
+ end do
+ !$omp end do
+
+end subroutine ordered_case
+
+! ORDERED(2) associates the inner loop with the directive as well, so it is
+! not wrapped either.
+! CHECK-LABEL: func.func @_QPordered_case(
+! CHECK: omp.wsloop ordered(2)
+! CHECK: omp.loop_nest
+! CHECK: cf.cond_br
+! CHECK: omp.yield
+
+! A TILE case belongs here too, since the SIZES arguments decide how many
+! loops are associated with the directive. It is left out for now because
+! lowering a TILE whose body is unstructured currently fails an assertion,
+! independently of whether wrapping is enabled:
+! https://github.com/llvm/llvm-project/issues/216701
More information about the flang-commits
mailing list