-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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.
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
Skip compiler directives between OMP PARALLEL DO and the loop #81021
Conversation
This fixes a compilation error when code like this is presented to the compiler: !$OMP PARALLEL DO !DIR$ VECTOR ALIGNED DO 20 i=1,N a = a + 0.5 20 CONTINUE The directive itself is later ignored (with a warning that this is happening), but because the compiler already errored out before that point, it completely fails to compile this code. Other compilers accept the code without complaints.
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-semantics Author: Mats Petersson (Leporacanthicus) ChangesThis fixes a compilation error when code like this is presented to the compiler: !$OMP PARALLEL DO The directive itself is later ignored (with a warning that this is happening), but because the compiler already errored out before that point, it completely fails to compile this code. Other compilers accept the code without complaints. Full diff: https://github.com/llvm/llvm-project/pull/81021.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/canonicalize-omp.cpp b/flang/lib/Semantics/canonicalize-omp.cpp
index 013fb408214eef..01adcf53728424 100644
--- a/flang/lib/Semantics/canonicalize-omp.cpp
+++ b/flang/lib/Semantics/canonicalize-omp.cpp
@@ -90,7 +90,11 @@ class CanonicalizationOfOmp {
auto &dir{std::get<parser::OmpLoopDirective>(beginDir.t)};
nextIt = it;
- if (++nextIt != block.end()) {
+ while (++nextIt != block.end()) {
+ // Ignore compiler directives.
+ if (auto *directive{GetConstructIf<parser::CompilerDirective>(*nextIt)})
+ continue;
+
if (auto *doCons{GetConstructIf<parser::DoConstruct>(*nextIt)}) {
if (doCons->GetLoopControl()) {
// move DoConstruct
@@ -111,12 +115,14 @@ class CanonicalizationOfOmp {
"DO loop after the %s directive must have loop control"_err_en_US,
parser::ToUpperCaseLetters(dir.source.ToString()));
}
- return; // found do-loop
+ } else {
+ messages_.Say(dir.source,
+ "A DO loop must follow the %s directive"_err_en_US,
+ parser::ToUpperCaseLetters(dir.source.ToString()));
}
+ // If we get here, we either found a loop, or issued an error message.
+ return;
}
- messages_.Say(dir.source,
- "A DO loop must follow the %s directive"_err_en_US,
- parser::ToUpperCaseLetters(dir.source.ToString()));
}
void RewriteOmpAllocations(parser::ExecutionPart &body) {
diff --git a/flang/test/Semantics/OpenMP/loop-association.f90 b/flang/test/Semantics/OpenMP/loop-association.f90
index 8a28fd8878f496..29fca3b4be5902 100644
--- a/flang/test/Semantics/OpenMP/loop-association.f90
+++ b/flang/test/Semantics/OpenMP/loop-association.f90
@@ -30,6 +30,13 @@
c = c - 1
END DO outer
+ ! Accept directives between parallel do and actual loop.
+ !$OMP PARALLEL DO
+ !DIR$ VECTOR ALIGNED
+ DO 20 i=1,N
+ a = a + 0.5
+20 CONTINUE
+
c = 16
!ERROR: DO loop after the PARALLEL DO directive must have loop control
!$omp parallel do
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this!
DO 20 i=1,N | ||
a = a + 0.5 | ||
20 CONTINUE | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this needs an !$OMP END PARALLEL DO
to be valid openmp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the worksharing loop directives, the closing END
directive is optional. However, I agree with the nit and we should have as a matter of style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add it, because it wasn't in the other tests that do similar loops. But I agree, it's nicer to have, so I have added it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please fix the warning in canonicalize-omp.cpp?
if (++nextIt != block.end()) { | ||
while (++nextIt != block.end()) { | ||
// Ignore compiler directives. | ||
if (auto *directive{GetConstructIf<parser::CompilerDirective>(*nextIt)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting a warning on this line because the variable directive
isn't used. And since I have -Werror
set, my build fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for that. Neither my GCC nor my Clang builds that I run warned for this - Trivial fix here:
This fixes a compilation error when code like this is presented to the compiler:
!$OMP PARALLEL DO
!DIR$ VECTOR ALIGNED
DO 20 i=1,N
a = a + 0.5
20 CONTINUE
The directive itself is later ignored (with a warning that this is happening), but because the compiler already errored out before that point, it completely fails to compile this code. Other compilers accept the code without complaints.