[flang-commits] [flang] [flang][OpenMP] move omp end directive validation to semantics (PR #154739)
Tom Eccles via flang-commits
flang-commits at lists.llvm.org
Tue Aug 26 02:47:54 PDT 2025
https://github.com/tblah updated https://github.com/llvm/llvm-project/pull/154739
>From 040a19d1277fa90c395f5181896144b56aea6b1e Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Wed, 20 Aug 2025 16:46:06 +0000
Subject: [PATCH 1/3] [flang][OpenMP] move omp end directive validation to
semantics
The old parse tree errors quckly exploded to thousands of unhelpful
lines when there were multiple missing end directives (e.g. #90452).
Instead I've added a flag to the parse tree indicating when a missing
end directive needs to be diagnosed, and moved the error messages to
semantics (where they are a lot easier to control).
This has the disadvantage of not displaying the error if there were
other parse errors, but there is a precedent for this approach (e.g.
parsing atomic constructs).
---
flang/include/flang/Parser/dump-parse-tree.h | 1 +
flang/include/flang/Parser/parse-tree.h | 12 +++-
flang/lib/Parser/openmp-parsers.cpp | 24 ++++++--
flang/lib/Semantics/check-omp-structure.cpp | 8 +++
flang/test/Parser/OpenMP/fail-construct1.f90 | 2 +-
.../OpenMP/ordered-block-vs-standalone.f90 | 60 +++++++++++++++++++
.../OpenMP/missing-end-directive.f90 | 13 ++++
7 files changed, 114 insertions(+), 6 deletions(-)
create mode 100644 flang/test/Parser/OpenMP/ordered-block-vs-standalone.f90
create mode 100644 flang/test/Semantics/OpenMP/missing-end-directive.f90
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 57421da4fa938..6bce8e8e5c00d 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -713,6 +713,7 @@ class ParseTreeDumper {
NODE(parser, OmpEndDirective)
NODE(parser, OpenMPAtomicConstruct)
NODE(parser, OpenMPBlockConstruct)
+ NODE_ENUM(OpenMPBlockConstruct, Flags)
NODE(parser, OpenMPCancelConstruct)
NODE(parser, OpenMPCancellationPointConstruct)
NODE(parser, OpenMPConstruct)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 1d1a4a163084b..38ec605574c06 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4783,15 +4783,25 @@ struct OmpEndDirective : public OmpDirectiveSpecification {
// Common base class for block-associated constructs.
struct OmpBlockConstruct {
TUPLE_CLASS_BOILERPLATE(OmpBlockConstruct);
+ ENUM_CLASS(Flags, None, MissingMandatoryEndDirective);
+
+ /// Constructor with defualt value for Flags.
+ OmpBlockConstruct(OmpBeginDirective &&begin, Block &&block,
+ std::optional<OmpEndDirective> &&end)
+ : t(std::move(begin), std::move(block), std::move(end), Flags::None) {}
+
const OmpBeginDirective &BeginDir() const {
return std::get<OmpBeginDirective>(t);
}
const std::optional<OmpEndDirective> &EndDir() const {
return std::get<std::optional<OmpEndDirective>>(t);
}
+ bool isMissingMandatoryEndDirecitive() const {
+ return std::get<Flags>(t) == Flags::MissingMandatoryEndDirective;
+ }
CharBlock source;
- std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>> t;
+ std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>, Flags> t;
};
struct OmpMetadirectiveDirective {
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 56cee4ab38e9b..9670302c8549b 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1474,6 +1474,7 @@ struct OmpBlockConstructParser {
constexpr OmpBlockConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
std::optional<resultType> Parse(ParseState &state) const {
+ using Flags = OmpBlockConstruct::Flags;
if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
// Try strictly-structured block with an optional end-directive
@@ -1484,11 +1485,26 @@ struct OmpBlockConstructParser {
[](auto &&s) { return OmpEndDirective(std::move(s)); })};
} else if (auto &&body{
attempt(LooselyStructuredBlockParser{}).Parse(state)}) {
- // Try loosely-structured block with a mandatory end-directive
- if (auto end{OmpEndDirectiveParser{dir_}.Parse(state)}) {
- return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
- std::move(*body), OmpEndDirective{std::move(*end)}};
+ // Try loosely-structured block with a mandatory end-directive.
+ auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
+ // Dereference outer optional (maybe() always succeeds) and look at the
+ // inner optional.
+ bool endPresent = end->has_value();
+
+ // ORDERED is special. We do need to return failure here so that the
+ // standalone ORDERED construct can be distinguished from the block
+ // associated construct.
+ if (!endPresent && dir_ == llvm::omp::Directive::OMPD_ordered) {
+ return std::nullopt;
}
+
+ // Delay the error for a missing end-directive until semantics so that
+ // we have better control over the output.
+ return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
+ std::move(*body),
+ llvm::transformOptional(std::move(*end),
+ [](auto &&s) { return OmpEndDirective(std::move(s)); }),
+ endPresent ? Flags::None : Flags::MissingMandatoryEndDirective};
}
}
return std::nullopt;
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 92a2cfc330d35..0bdd2c62f88ce 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -785,6 +785,14 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
const parser::Block &block{std::get<parser::Block>(x.t)};
PushContextAndClauseSets(beginSpec.DirName().source, beginSpec.DirId());
+
+ // Missing mandatory end block: this is checked in semantics because that
+ // makes it easier to control the error messages.
+ if (x.isMissingMandatoryEndDirecitive()) {
+ context_.Say(
+ x.BeginDir().source, "Expected OpenMP end directive"_err_en_US);
+ }
+
if (llvm::omp::allTargetSet.test(GetContext().directive)) {
EnterDirectiveNest(TargetNest);
}
diff --git a/flang/test/Parser/OpenMP/fail-construct1.f90 b/flang/test/Parser/OpenMP/fail-construct1.f90
index f0b3f7438ae58..9d1af903344d3 100644
--- a/flang/test/Parser/OpenMP/fail-construct1.f90
+++ b/flang/test/Parser/OpenMP/fail-construct1.f90
@@ -1,5 +1,5 @@
! RUN: not %flang_fc1 -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
!$omp parallel
-! CHECK: error: expected '!$OMP '
+! CHECK: error: Expected OpenMP end directive
end
diff --git a/flang/test/Parser/OpenMP/ordered-block-vs-standalone.f90 b/flang/test/Parser/OpenMP/ordered-block-vs-standalone.f90
new file mode 100644
index 0000000000000..db9c45add8674
--- /dev/null
+++ b/flang/test/Parser/OpenMP/ordered-block-vs-standalone.f90
@@ -0,0 +1,60 @@
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp -fopenmp-version=45 %s | FileCheck %s
+
+! Check that standalone ORDERED is successfully distinguished form block associated ORDERED
+
+! CHECK: | SubroutineStmt
+! CHECK-NEXT: | | Name = 'standalone'
+subroutine standalone
+ integer :: x(10, 10)
+ do i = 1, 10
+ do j = 1,10
+ ! CHECK: OpenMPConstruct -> OpenMPStandaloneConstruct
+ ! CHECK-NEXT: | OmpDirectiveName -> llvm::omp::Directive = ordered
+ ! CHECK-NEXT: | OmpClauseList ->
+ ! CHECK-NEXT: | Flags = None
+ !$omp ordered
+ x(i, j) = i + j
+ end do
+ end do
+endsubroutine
+
+! CHECK: | SubroutineStmt
+! CHECK-NEXT: | | Name = 'strict_block'
+subroutine strict_block
+ integer :: x(10, 10)
+ integer :: tmp
+ do i = 1, 10
+ do j = 1,10
+ ! CHECK: OpenMPConstruct -> OpenMPBlockConstruct
+ ! CHECK-NEXT: | OmpBeginDirective
+ ! CHECK-NEXT: | | OmpDirectiveName -> llvm::omp::Directive = ordered
+ ! CHECK-NEXT: | | OmpClauseList ->
+ ! CHECK-NEXT: | | Flags = None
+ !$omp ordered
+ block
+ tmp = i + j
+ x(i, j) = tmp
+ end block
+ end do
+ end do
+endsubroutine
+
+! CHECK: | SubroutineStmt
+! CHECK-NEXT: | | Name = 'loose_block'
+subroutine loose_block
+ integer :: x(10, 10)
+ integer :: tmp
+ do i = 1, 10
+ do j = 1,10
+ ! CHECK: OpenMPConstruct -> OpenMPBlockConstruct
+ ! CHECK-NEXT: | OmpBeginDirective
+ ! CHECK-NEXT: | | OmpDirectiveName -> llvm::omp::Directive = ordered
+ ! CHECK-NEXT: | | OmpClauseList ->
+ ! CHECK-NEXT: | | Flags = None
+ !$omp ordered
+ tmp = i + j
+ x(i, j) = tmp
+ !$omp end ordered
+ end do
+ end do
+endsubroutine
diff --git a/flang/test/Semantics/OpenMP/missing-end-directive.f90 b/flang/test/Semantics/OpenMP/missing-end-directive.f90
new file mode 100644
index 0000000000000..3b870d134155b
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/missing-end-directive.f90
@@ -0,0 +1,13 @@
+! RUN: %python %S/../test_errors.py %s %flang -fopenmp
+
+! Test that we can diagnose missing end directives without an explosion of errors
+
+! ERROR: Expected OpenMP end directive
+!$omp parallel
+! ERROR: Expected OpenMP end directive
+!$omp task
+! ERROR: Expected OpenMP end directive
+!$omp parallel
+! ERROR: Expected OpenMP end directive
+!$omp task
+end
>From 16662ce212a4bbb3a10065897c28dac1df1a7d40 Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Thu, 21 Aug 2025 15:46:19 +0000
Subject: [PATCH 2/3] Check for loosely-strictured block instead of maintaining
a flag
---
flang/include/flang/Parser/dump-parse-tree.h | 1 -
flang/include/flang/Parser/parse-tree.h | 12 +-----------
flang/lib/Parser/openmp-parsers.cpp | 4 +---
flang/lib/Semantics/check-omp-structure.cpp | 18 +++++++++++++++++-
4 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 6bce8e8e5c00d..57421da4fa938 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -713,7 +713,6 @@ class ParseTreeDumper {
NODE(parser, OmpEndDirective)
NODE(parser, OpenMPAtomicConstruct)
NODE(parser, OpenMPBlockConstruct)
- NODE_ENUM(OpenMPBlockConstruct, Flags)
NODE(parser, OpenMPCancelConstruct)
NODE(parser, OpenMPCancellationPointConstruct)
NODE(parser, OpenMPConstruct)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 38ec605574c06..1d1a4a163084b 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4783,25 +4783,15 @@ struct OmpEndDirective : public OmpDirectiveSpecification {
// Common base class for block-associated constructs.
struct OmpBlockConstruct {
TUPLE_CLASS_BOILERPLATE(OmpBlockConstruct);
- ENUM_CLASS(Flags, None, MissingMandatoryEndDirective);
-
- /// Constructor with defualt value for Flags.
- OmpBlockConstruct(OmpBeginDirective &&begin, Block &&block,
- std::optional<OmpEndDirective> &&end)
- : t(std::move(begin), std::move(block), std::move(end), Flags::None) {}
-
const OmpBeginDirective &BeginDir() const {
return std::get<OmpBeginDirective>(t);
}
const std::optional<OmpEndDirective> &EndDir() const {
return std::get<std::optional<OmpEndDirective>>(t);
}
- bool isMissingMandatoryEndDirecitive() const {
- return std::get<Flags>(t) == Flags::MissingMandatoryEndDirective;
- }
CharBlock source;
- std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>, Flags> t;
+ std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>> t;
};
struct OmpMetadirectiveDirective {
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 9670302c8549b..2093aca38c9d6 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1474,7 +1474,6 @@ struct OmpBlockConstructParser {
constexpr OmpBlockConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
std::optional<resultType> Parse(ParseState &state) const {
- using Flags = OmpBlockConstruct::Flags;
if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
// Try strictly-structured block with an optional end-directive
@@ -1503,8 +1502,7 @@ struct OmpBlockConstructParser {
return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
std::move(*body),
llvm::transformOptional(std::move(*end),
- [](auto &&s) { return OmpEndDirective(std::move(s)); }),
- endPresent ? Flags::None : Flags::MissingMandatoryEndDirective};
+ [](auto &&s) { return OmpEndDirective(std::move(s)); })};
}
}
return std::nullopt;
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 0bdd2c62f88ce..835802d81894e 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -788,7 +788,23 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
// Missing mandatory end block: this is checked in semantics because that
// makes it easier to control the error messages.
- if (x.isMissingMandatoryEndDirecitive()) {
+ // The end block is mandatory when the construct is not applied to a strictly
+ // structured block (aka it is applied to a loosely structured block). In
+ // other words, the body doesn't contain exactly one parser::BlockConstruct.
+ auto isStrictlyStructuredBlock{[](const parser::Block &block) -> bool {
+ if (block.size() != 1) {
+ return false;
+ }
+ const parser::ExecutionPartConstruct &contents{block.front()};
+ auto *executableConstruct{
+ std::get_if<parser::ExecutableConstruct>(&contents.u)};
+ if (!executableConstruct) {
+ return false;
+ }
+ return std::holds_alternative<common::Indirection<parser::BlockConstruct>>(
+ executableConstruct->u);
+ }};
+ if (!endSpec && !isStrictlyStructuredBlock(block)) {
context_.Say(
x.BeginDir().source, "Expected OpenMP end directive"_err_en_US);
}
>From 85d81c5c059a4af9947295732d760b34046e0bd0 Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Tue, 26 Aug 2025 09:44:01 +0000
Subject: [PATCH 3/3] Use braced initialization
---
flang/lib/Parser/openmp-parsers.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 2093aca38c9d6..65395c50c4ddf 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -1488,7 +1488,7 @@ struct OmpBlockConstructParser {
auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
// Dereference outer optional (maybe() always succeeds) and look at the
// inner optional.
- bool endPresent = end->has_value();
+ bool endPresent{end->has_value()};
// ORDERED is special. We do need to return failure here so that the
// standalone ORDERED construct can be distinguished from the block
More information about the flang-commits
mailing list