From 040a19d1277fa90c395f5181896144b56aea6b1e Mon Sep 17 00:00:00 2001 From: Tom Eccles 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 &&end) + : t(std::move(begin), std::move(block), std::move(end), Flags::None) {} + const OmpBeginDirective &BeginDir() const { return std::get(t); } const std::optional &EndDir() const { return std::get>(t); } + bool isMissingMandatoryEndDirecitive() const { + return std::get(t) == Flags::MissingMandatoryEndDirective; + } CharBlock source; - std::tuple> t; + std::tuple, 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 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(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 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 &&end) - : t(std::move(begin), std::move(block), std::move(end), Flags::None) {} - const OmpBeginDirective &BeginDir() const { return std::get(t); } const std::optional &EndDir() const { return std::get>(t); } - bool isMissingMandatoryEndDirecitive() const { - return std::get(t) == Flags::MissingMandatoryEndDirective; - } CharBlock source; - std::tuple, Flags> t; + std::tuple> 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 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(&contents.u)}; + if (!executableConstruct) { + return false; + } + return std::holds_alternative>( + 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 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