Skip to content

[flang][OpenMP] Parse ORDERED as standalone when DEPEND/DOACROSS is p… #156693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 4, 2025
Merged

[flang][OpenMP] Parse ORDERED as standalone when DEPEND/DOACROSS is p… #156693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions flang/lib/Parser/openmp-parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,16 @@ static bool IsFortranBlockConstruct(const ExecutionPartConstruct &epc) {
}
}

static bool IsStandaloneOrdered(const OmpDirectiveSpecification &dirSpec) {
// An ORDERED construct is standalone if it has DOACROSS or DEPEND clause.
return dirSpec.DirId() == llvm::omp::Directive::OMPD_ordered &&
llvm::any_of(dirSpec.Clauses().v, [](const OmpClause &clause) {
llvm::omp::Clause id{clause.Id()};
return id == llvm::omp::Clause::OMPC_depend ||
id == llvm::omp::Clause::OMPC_doacross;
});
}

struct StrictlyStructuredBlockParser {
using resultType = Block;

Expand Down Expand Up @@ -1475,6 +1485,9 @@ struct OmpBlockConstructParser {

std::optional<resultType> Parse(ParseState &state) const {
if (auto &&begin{OmpBeginDirectiveParser(dir_).Parse(state)}) {
if (IsStandaloneOrdered(*begin)) {
return std::nullopt;
}
if (auto &&body{attempt(StrictlyStructuredBlockParser{}).Parse(state)}) {
// Try strictly-structured block with an optional end-directive
auto end{maybe(OmpEndDirectiveParser{dir_}).Parse(state)};
Expand All @@ -1486,17 +1499,6 @@ struct OmpBlockConstructParser {
attempt(LooselyStructuredBlockParser{}).Parse(state)}) {
// 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)),
Expand Down Expand Up @@ -1653,7 +1655,6 @@ TYPE_PARSER(sourced( //
static bool IsSimpleStandalone(const OmpDirectiveName &name) {
switch (name.v) {
case llvm::omp::Directive::OMPD_barrier:
case llvm::omp::Directive::OMPD_ordered:
case llvm::omp::Directive::OMPD_scan:
case llvm::omp::Directive::OMPD_target_enter_data:
case llvm::omp::Directive::OMPD_target_exit_data:
Expand All @@ -1669,7 +1670,9 @@ static bool IsSimpleStandalone(const OmpDirectiveName &name) {
TYPE_PARSER(sourced( //
construct<OpenMPSimpleStandaloneConstruct>(
predicated(OmpDirectiveNameParser{}, IsSimpleStandalone) >=
Parser<OmpDirectiveSpecification>{})))
Parser<OmpDirectiveSpecification>{}) ||
construct<OpenMPSimpleStandaloneConstruct>(
predicated(Parser<OmpDirectiveSpecification>{}, IsStandaloneOrdered))))

TYPE_PARSER(sourced( //
construct<OpenMPFlushConstruct>(
Expand Down
12 changes: 10 additions & 2 deletions flang/lib/Semantics/check-omp-structure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,8 +890,16 @@ void OmpStructureChecker::Enter(const parser::OmpBlockConstruct &x) {
executableConstruct->u);
}};
if (!endSpec && !isStrictlyStructuredBlock(block)) {
context_.Say(
x.BeginDir().source, "Expected OpenMP end directive"_err_en_US);
llvm::omp::Directive dirId{beginSpec.DirId()};
auto &msg{context_.Say(beginSpec.source,
"Expected OpenMP END %s directive"_err_en_US,
parser::ToUpperCaseLetters(getDirectiveName(dirId)))};
// ORDERED has two variants, so be explicit about which variant we think
// this is.
if (dirId == llvm::omp::Directive::OMPD_ordered) {
msg.Attach(
beginSpec.source, "The ORDERED directive is block-associated"_en_US);
}
}

if (llvm::omp::allTargetSet.test(GetContext().directive)) {
Expand Down
4 changes: 2 additions & 2 deletions flang/test/Parser/OpenMP/fail-construct1.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
! RUN: not %flang_fc1 -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s

!$omp parallel
! CHECK: error: Expected OpenMP end directive
!$omp parallel
! CHECK: error: Expected OpenMP END PARALLEL directive
end
4 changes: 2 additions & 2 deletions flang/test/Parser/OpenMP/ordered-block-vs-standalone.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp -fopenmp-version=45 %s | FileCheck %s
! RUN: %flang_fc1 -fdebug-dump-parse-tree-no-sema -fopenmp -fopenmp-version=45 %s | FileCheck %s

! Check that standalone ORDERED is successfully distinguished form block associated ORDERED

Expand All @@ -12,7 +12,7 @@ subroutine standalone
! CHECK-NEXT: | OmpDirectiveName -> llvm::omp::Directive = ordered
! CHECK-NEXT: | OmpClauseList ->
! CHECK-NEXT: | Flags = None
!$omp ordered
!$omp ordered depend(source)
x(i, j) = i + j
end do
end do
Expand Down
8 changes: 4 additions & 4 deletions flang/test/Semantics/OpenMP/missing-end-directive.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@

! Test that we can diagnose missing end directives without an explosion of errors

! ERROR: Expected OpenMP end directive
! ERROR: Expected OpenMP END PARALLEL directive
!$omp parallel
! ERROR: Expected OpenMP end directive
! ERROR: Expected OpenMP END TASK directive
!$omp task
! ERROR: Expected OpenMP END SECTIONS directive
!$omp sections
! ERROR: Expected OpenMP end directive
! ERROR: Expected OpenMP END PARALLEL directive
!$omp parallel
! ERROR: Expected OpenMP end directive
! ERROR: Expected OpenMP END TASK directive
!$omp task
! ERROR: Expected OpenMP END SECTIONS directive
!$omp sections
Expand Down
17 changes: 2 additions & 15 deletions flang/test/Semantics/OpenMP/ordered01.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,27 +52,14 @@ program main
end do
!$omp end do

!$omp do ordered(1)
do i = 2, N
!ERROR: DEPEND clauses are not allowed when ORDERED construct is a block construct with an ORDERED region
!$omp ordered depend(source)
arrayA(i) = foo(i)
!$omp end ordered
Copy link
Contributor Author

@kparzysz kparzysz Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will now cause a parser error: "expected END". It's not very descriptive, but it's the same message for all unmatched end directives.

Copy link
Contributor

@tblah tblah Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could continue the pattern of parsing approximately and then generating better errors in semantics. Here we could allow stray OmpEndDirective as an OpenMPConstruct (lowest priority). I spent some time messing with it but while I could detect the error condition in parsing, I didn't find a sufficiently non-hacky way to print the error message because the parser doesn't distinguish between "this didn't parse" and "there is an error here".

Copy link
Contributor Author

@kparzysz kparzysz Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a couple of attempts to detect a "stray" end directive in the parser, but none of them worked. I've concluded that we'd need to use some form of the recovery parser to make sure the diagnostic message is printed. The recovery parser takes two parser arguments: (1) the "normal" parser, and (2) the parser that tries to do the recovery when (1) fails. When (2) succeeds, the parser is in recovery mode, the parsing continues, but will fail eventually because of the failure of (1).

When we parse parser::Block (i.e. a list of executable constructs), we rely on the fact that it will stop when it gets something that is not an executable construct. For us an end-directive fulfills that role: we parse "block", then, when that's done, we expect the end-directive. However, if we put the recovery parser in place, it will be invoked (from inside of the block parser) when an end-directive is encountered. So the block parsing won't just end gracefully on an end directive, it will hit the recovery parser and fail every single time. So that is clearly not good...

One more thing I want to try was to parse parser::Block "manually" with preemptively checking for an end directive, i.e. something like

loop:
  if next_thing != end-directive:
    exec_construct = parse(next_thing)
    body.append(exec_construct)
  else:
    stop
end loop

This way we will always handle all the expected end directives by ourselves. The recovery parser would only see the misplaced ones.

!ERROR: DEPEND clauses are not allowed when ORDERED construct is a block construct with an ORDERED region
!$omp ordered depend(sink: i - 1)
arrayB(i) = bar(arrayA(i), arrayB(i-1))
!$omp end ordered
end do
!$omp end do

contains
subroutine work1()
!ERROR: THREADS and SIMD clauses are not allowed when ORDERED construct is a standalone construct with no ORDERED region
!ERROR: Expected OpenMP END ORDERED directive
!$omp ordered simd
end subroutine work1

subroutine work2()
!ERROR: THREADS and SIMD clauses are not allowed when ORDERED construct is a standalone construct with no ORDERED region
!ERROR: Expected OpenMP END ORDERED directive
!$omp ordered threads
end subroutine work2

Expand Down