Skip to content

Commit 040a19d

Browse files
committed
[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).
1 parent 60ee056 commit 040a19d

File tree

7 files changed

+114
-6
lines changed

7 files changed

+114
-6
lines changed

flang/include/flang/Parser/dump-parse-tree.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,7 @@ class ParseTreeDumper {
713713
NODE(parser, OmpEndDirective)
714714
NODE(parser, OpenMPAtomicConstruct)
715715
NODE(parser, OpenMPBlockConstruct)
716+
NODE_ENUM(OpenMPBlockConstruct, Flags)
716717
NODE(parser, OpenMPCancelConstruct)
717718
NODE(parser, OpenMPCancellationPointConstruct)
718719
NODE(parser, OpenMPConstruct)

flang/include/flang/Parser/parse-tree.h

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4783,15 +4783,25 @@ struct OmpEndDirective : public OmpDirectiveSpecification {
47834783
// Common base class for block-associated constructs.
47844784
struct OmpBlockConstruct {
47854785
TUPLE_CLASS_BOILERPLATE(OmpBlockConstruct);
4786+
ENUM_CLASS(Flags, None, MissingMandatoryEndDirective);
4787+
4788+
/// Constructor with defualt value for Flags.
4789+
OmpBlockConstruct(OmpBeginDirective &&begin, Block &&block,
4790+
std::optional<OmpEndDirective> &&end)
4791+
: t(std::move(begin), std::move(block), std::move(end), Flags::None) {}
4792+
47864793
const OmpBeginDirective &BeginDir() const {
47874794
return std::get<OmpBeginDirective>(t);
47884795
}
47894796
const std::optional<OmpEndDirective> &EndDir() const {
47904797
return std::get<std::optional<OmpEndDirective>>(t);
47914798
}
4799+
bool isMissingMandatoryEndDirecitive() const {
4800+
return std::get<Flags>(t) == Flags::MissingMandatoryEndDirective;
4801+
}
47924802

47934803
CharBlock source;
4794-
std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>> t;
4804+
std::tuple<OmpBeginDirective, Block, std::optional<OmpEndDirective>, Flags> t;
47954805
};
47964806

47974807
struct OmpMetadirectiveDirective {

flang/lib/Parser/openmp-parsers.cpp

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,6 +1474,7 @@ struct OmpBlockConstructParser {
14741474
constexpr OmpBlockConstructParser(llvm::omp::Directive dir) : dir_(dir) {}
14751475

14761476
std::optional<resultType> Parse(ParseState &state) const {
1477+
using Flags = OmpBlockConstruct::Flags;
14771478
if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
14781479
if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
14791480
// Try strictly-structured block with an optional end-directive
@@ -1484,11 +1485,26 @@ struct OmpBlockConstructParser {
14841485
[](auto &&s) { return OmpEndDirective(std::move(s)); })};
14851486
} else if (auto &&body{
14861487
attempt(LooselyStructuredBlockParser{}).Parse(state)}) {
1487-
// Try loosely-structured block with a mandatory end-directive
1488-
if (auto end{OmpEndDirectiveParser{dir_}.Parse(state)}) {
1489-
return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
1490-
std::move(*body), OmpEndDirective{std::move(*end)}};
1488+
// Try loosely-structured block with a mandatory end-directive.
1489+
auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
1490+
// Dereference outer optional (maybe() always succeeds) and look at the
1491+
// inner optional.
1492+
bool endPresent = end->has_value();
1493+
1494+
// ORDERED is special. We do need to return failure here so that the
1495+
// standalone ORDERED construct can be distinguished from the block
1496+
// associated construct.
1497+
if (!endPresent && dir_ == llvm::omp::Directive::OMPD_ordered) {
1498+
return std::nullopt;
14911499
}
1500+
1501+
// Delay the error for a missing end-directive until semantics so that
1502+
// we have better control over the output.
1503+
return OmpBlockConstruct{OmpBeginDirective(std::move(*begin)),
1504+
std::move(*body),
1505+
llvm::transformOptional(std::move(*end),
1506+
[](auto &&s) { return OmpEndDirective(std::move(s)); }),
1507+
endPresent ? Flags::None : Flags::MissingMandatoryEndDirective};
14921508
}
14931509
}
14941510
return std::nullopt;

flang/lib/Semantics/check-omp-structure.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,14 @@ void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
785785
const parser::Block &block{std::get<parser::Block>(x.t)};
786786

787787
PushContextAndClauseSets(beginSpec.DirName().source, beginSpec.DirId());
788+
789+
// Missing mandatory end block: this is checked in semantics because that
790+
// makes it easier to control the error messages.
791+
if (x.isMissingMandatoryEndDirecitive()) {
792+
context_.Say(
793+
x.BeginDir().source, "Expected OpenMP end directive"_err_en_US);
794+
}
795+
788796
if (llvm::omp::allTargetSet.test(GetContext().directive)) {
789797
EnterDirectiveNest(TargetNest);
790798
}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
! RUN: not %flang_fc1 -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
22

33
!$omp parallel
4-
! CHECK: error: expected '!$OMP '
4+
! CHECK: error: Expected OpenMP end directive
55
end
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp -fopenmp-version=45 %s | FileCheck %s
2+
3+
! Check that standalone ORDERED is successfully distinguished form block associated ORDERED
4+
5+
! CHECK: | SubroutineStmt
6+
! CHECK-NEXT: | | Name = 'standalone'
7+
subroutine standalone
8+
integer :: x(10, 10)
9+
do i = 1, 10
10+
do j = 1,10
11+
! CHECK: OpenMPConstruct -> OpenMPStandaloneConstruct
12+
! CHECK-NEXT: | OmpDirectiveName -> llvm::omp::Directive = ordered
13+
! CHECK-NEXT: | OmpClauseList ->
14+
! CHECK-NEXT: | Flags = None
15+
!$omp ordered
16+
x(i, j) = i + j
17+
end do
18+
end do
19+
endsubroutine
20+
21+
! CHECK: | SubroutineStmt
22+
! CHECK-NEXT: | | Name = 'strict_block'
23+
subroutine strict_block
24+
integer :: x(10, 10)
25+
integer :: tmp
26+
do i = 1, 10
27+
do j = 1,10
28+
! CHECK: OpenMPConstruct -> OpenMPBlockConstruct
29+
! CHECK-NEXT: | OmpBeginDirective
30+
! CHECK-NEXT: | | OmpDirectiveName -> llvm::omp::Directive = ordered
31+
! CHECK-NEXT: | | OmpClauseList ->
32+
! CHECK-NEXT: | | Flags = None
33+
!$omp ordered
34+
block
35+
tmp = i + j
36+
x(i, j) = tmp
37+
end block
38+
end do
39+
end do
40+
endsubroutine
41+
42+
! CHECK: | SubroutineStmt
43+
! CHECK-NEXT: | | Name = 'loose_block'
44+
subroutine loose_block
45+
integer :: x(10, 10)
46+
integer :: tmp
47+
do i = 1, 10
48+
do j = 1,10
49+
! CHECK: OpenMPConstruct -> OpenMPBlockConstruct
50+
! CHECK-NEXT: | OmpBeginDirective
51+
! CHECK-NEXT: | | OmpDirectiveName -> llvm::omp::Directive = ordered
52+
! CHECK-NEXT: | | OmpClauseList ->
53+
! CHECK-NEXT: | | Flags = None
54+
!$omp ordered
55+
tmp = i + j
56+
x(i, j) = tmp
57+
!$omp end ordered
58+
end do
59+
end do
60+
endsubroutine
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
! RUN: %python %S/../test_errors.py %s %flang -fopenmp
2+
3+
! Test that we can diagnose missing end directives without an explosion of errors
4+
5+
! ERROR: Expected OpenMP end directive
6+
!$omp parallel
7+
! ERROR: Expected OpenMP end directive
8+
!$omp task
9+
! ERROR: Expected OpenMP end directive
10+
!$omp parallel
11+
! ERROR: Expected OpenMP end directive
12+
!$omp task
13+
end

0 commit comments

Comments
 (0)