[flang][OpenMP] Reject END DO on construct that crosses label-DO #169714
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The idea is that there can be multiple flags on a given directive. When "Flags" was a simple enum, only one flag could have been set at a time.
In a label-DO construct where two or more loops share the same teminator,
an OpenMP construct must enclose all the loops if an end-directive is
present. E.g.
```
do 100 i = 1,10
!$omp do
do 100 j = 1,10
100 continue
!$omp end do ! Error, but ok if this line is removed
```
OpenMP 5.1 and later no longer has this restriction.
Fixes #169536.
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-parser @llvm/pr-subscribers-flang-semantics Author: Krzysztof Parzyszek (kparzysz) ChangesIn a label-DO construct where two or more loops share the same teminator, an OpenMP construct must enclose all the loops if an end-directive is present. E.g. OpenMP 5.1 and later no longer has this restriction. Fixes #169536. Full diff: https://github.com/llvm/llvm-project/pull/169714.diff 6 Files Affected:
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index dd928e1244a2f..e5d3d3f2a7d5b 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -4976,7 +4976,7 @@ struct OmpClauseList {
// --- Directives and constructs
struct OmpDirectiveSpecification {
- ENUM_CLASS(Flag, DeprecatedSyntax)
+ ENUM_CLASS(Flag, DeprecatedSyntax, CrossesLabelDo)
using Flags = common::EnumSet<Flag, Flag_enumSize>;
TUPLE_CLASS_BOILERPLATE(OmpDirectiveSpecification);
diff --git a/flang/lib/Semantics/canonicalize-do.cpp b/flang/lib/Semantics/canonicalize-do.cpp
index 409195d5960b4..a0a6f8d870f6e 100644
--- a/flang/lib/Semantics/canonicalize-do.cpp
+++ b/flang/lib/Semantics/canonicalize-do.cpp
@@ -92,8 +92,11 @@ class CanonicalizationOfDoLoops {
[&](common::Indirection<OpenMPConstruct> &construct) {
// If the body of the OpenMP construct ends with a label,
// treat the label as ending the construct itself.
- CanonicalizeIfMatch(
- block, stack, i, omp::GetFinalLabel(construct.value()));
+ OpenMPConstruct &omp{construct.value()};
+ if (CanonicalizeIfMatch(
+ block, stack, i, omp::GetFinalLabel(omp))) {
+ MarkOpenMPConstruct(omp);
+ }
},
},
executableConstruct->u);
@@ -103,12 +106,12 @@ class CanonicalizationOfDoLoops {
private:
template <typename T>
- void CanonicalizeIfMatch(Block &originalBlock, std::vector<LabelInfo> &stack,
+ bool CanonicalizeIfMatch(Block &originalBlock, std::vector<LabelInfo> &stack,
Block::iterator &i, Statement<T> &statement) {
- CanonicalizeIfMatch(originalBlock, stack, i, statement.label);
+ return CanonicalizeIfMatch(originalBlock, stack, i, statement.label);
}
- void CanonicalizeIfMatch(Block &originalBlock, std::vector<LabelInfo> &stack,
+ bool CanonicalizeIfMatch(Block &originalBlock, std::vector<LabelInfo> &stack,
Block::iterator &i, std::optional<Label> label) {
if (!stack.empty() && label && stack.back().label == *label) {
auto currentLabel{stack.back().label};
@@ -141,8 +144,27 @@ class CanonicalizationOfDoLoops {
stack.pop_back();
} while (!stack.empty() && stack.back().label == currentLabel);
i = --next;
+ return true;
+ } else {
+ return false;
}
}
+
+ void MarkOpenMPConstruct(OpenMPConstruct &omp) {
+ common::visit(
+ [](const auto &s) {
+ using S = std::decay_t<decltype(s)>;
+ if constexpr (std::is_base_of_v<OmpBlockConstruct, S> ||
+ std::is_same_v<OpenMPLoopConstruct, S>) {
+ const OmpDirectiveSpecification &beginSpec{s.BeginDir()};
+ auto &flags{
+ std::get<OmpDirectiveSpecification::Flags>(beginSpec.t)};
+ const_cast<OmpDirectiveSpecification::Flags &>(flags).set(
+ OmpDirectiveSpecification::Flag::CrossesLabelDo);
+ }
+ },
+ omp.u);
+ }
};
bool CanonicalizeDo(Program &program) {
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index 9a78209369949..5830bff1596b3 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -289,6 +289,23 @@ void OmpStructureChecker::CheckNestedBlock(const parser::OpenMPLoopConstruct &x,
void OmpStructureChecker::CheckNestedConstruct(
const parser::OpenMPLoopConstruct &x) {
size_t nestedCount{0};
+ unsigned version{context_.langOptions().OpenMPVersion};
+
+ if (version <= 50) {
+ const parser::OmpDirectiveSpecification &beginSpec{x.BeginDir()};
+ auto &flags{
+ std::get<parser::OmpDirectiveSpecification::Flags>(beginSpec.t)};
+ if (flags.test(parser::OmpDirectiveSpecification::Flag::CrossesLabelDo)) {
+ if (auto &endSpec{x.EndDir()}) {
+ parser::CharBlock beginSource{beginSpec.DirName().source};
+ context_
+ .Say(endSpec->DirName().source,
+ "END %s directive is not allowed when the construct does not contain all loops that shares a loop-terminating statement"_err_en_US,
+ parser::ToUpperCaseLetters(beginSource.ToString()))
+ .Attach(beginSource, "The construct starts here"_en_US);
+ }
+ }
+ }
auto &body{std::get<parser::Block>(x.t)};
if (body.empty()) {
diff --git a/flang/test/Parser/OpenMP/atomic-label-do.f90 b/flang/test/Parser/OpenMP/atomic-label-do.f90
index f0c83c01f7a21..1c7037f239191 100644
--- a/flang/test/Parser/OpenMP/atomic-label-do.f90
+++ b/flang/test/Parser/OpenMP/atomic-label-do.f90
@@ -29,7 +29,7 @@ subroutine f
!PARSE-TREE: | | | OmpBeginDirective
!PARSE-TREE: | | | | OmpDirectiveName -> llvm::omp::Directive = atomic
!PARSE-TREE: | | | | OmpClauseList -> OmpClause -> Write
-!PARSE-TREE: | | | | Flags = {}
+!PARSE-TREE: | | | | Flags = {CrossesLabelDo}
!PARSE-TREE: | | | Block
!PARSE-TREE: | | | | ExecutionPartConstruct -> ExecutableConstruct -> ActionStmt -> AssignmentStmt = 'x=i'
!PARSE-TREE: | | | | | Variable = 'x'
diff --git a/flang/test/Parser/OpenMP/cross-label-do.f90 b/flang/test/Parser/OpenMP/cross-label-do.f90
index fd648e0248258..c3768f8551c26 100644
--- a/flang/test/Parser/OpenMP/cross-label-do.f90
+++ b/flang/test/Parser/OpenMP/cross-label-do.f90
@@ -1,5 +1,5 @@
-!RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case --check-prefix="UNPARSE" %s
-!RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+!RUN: %flang_fc1 -fdebug-unparse -fopenmp -fopenmp-version=51 %s | FileCheck --ignore-case --check-prefix="UNPARSE" %s
+!RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp -fopenmp-version=51 %s | FileCheck --check-prefix="PARSE-TREE" %s
subroutine f00
integer :: i, j
@@ -7,6 +7,7 @@ subroutine f00
!$omp do
do 100 j = 1,10
100 continue
+!$omp end do ! This is legal in OpenMP 5.1+
end
!UNPARSE: SUBROUTINE f00
@@ -32,7 +33,7 @@ subroutine f00
!PARSE-TREE: | | | OmpBeginLoopDirective
!PARSE-TREE: | | | | OmpDirectiveName -> llvm::omp::Directive = do
!PARSE-TREE: | | | | OmpClauseList ->
-!PARSE-TREE: | | | | Flags = {}
+!PARSE-TREE: | | | | Flags = {CrossesLabelDo}
!PARSE-TREE: | | | Block
!PARSE-TREE: | | | | ExecutionPartConstruct -> ExecutableConstruct -> DoConstruct
!PARSE-TREE: | | | | | NonLabelDoStmt
diff --git a/flang/test/Semantics/OpenMP/loop-association.f90 b/flang/test/Semantics/OpenMP/loop-association.f90
index 4e63cafb3fda1..d39b9ed7b82c2 100644
--- a/flang/test/Semantics/OpenMP/loop-association.f90
+++ b/flang/test/Semantics/OpenMP/loop-association.f90
@@ -64,6 +64,8 @@
do 100 j=1, N
a = 3.14
100 continue
+ !This is only an error in OpenMP 5.0-.
+ !ERROR: END DO directive is not allowed when the construct does not contain all loops that shares a loop-terminating statement
!$omp enddo
!ERROR: Non-THREADPRIVATE object 'a' in COPYIN clause
|
Contributor
Author
tblah
approved these changes
Nov 27, 2025
Contributor
tblah
left a comment
tblah
left a comment
There was a problem hiding this comment.
LGTM.
I found the CanonicalizeIfMatch function pretty difficult to reason about, but that wasn't added in this commit and it might just be me so it can stay how it is. From the tests included in the PR and my own manual testing I am satisfied that this does do the right thing.
| } | ||
| } | ||
|
|
||
| void MarkOpenMPConstruct(OpenMPConstruct &omp) { |
Contributor
There was a problem hiding this comment.
nit: this can currently only mark as CrossesLabelDo and it does that unconditionally - which doesn't really fit the name. How about having an argument for the flag which should be set? (in this case, always CrossesLabelDo). I think that would make the calling code a bit easier to read as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a label-DO construct where two or more loops share the same teminator, an OpenMP construct must enclose all the loops if an end-directive is present. E.g.
OpenMP 5.1 and later no longer has this restriction.
Fixes #169536.