[flang][OpenMP] Tolerate compiler directives in loop constructs #169346
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
PR168884 flagged compiler directives (!dir$ ...) inside OpenMP loop constructs as errors. This caused some customer applications to fail to compile (issue 169229). Downgrade the error to a warning, and gracefully ignore compiler directives when lowering loop constructs to MLIR. Fixes #169229
kparzysz
requested review from
ergawy,
eugeneepshteyn,
kiranchandramohan and
skatrak
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-semantics @llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesPR168884 flagged compiler directives (!dir$ ...) inside OpenMP loop constructs as errors. This caused some customer applications to fail to compile (issue 169229). Downgrade the error to a warning, and gracefully ignore compiler directives when lowering loop constructs to MLIR. Fixes #169229 Full diff: https://github.com/llvm/llvm-project/pull/169346.diff 5 Files Affected:
diff --git a/flang/lib/Lower/OpenMP/Utils.cpp b/flang/lib/Lower/OpenMP/Utils.cpp
index 7d7a4869ab3a6..4af6fb3e88f84 100644
--- a/flang/lib/Lower/OpenMP/Utils.cpp
+++ b/flang/lib/Lower/OpenMP/Utils.cpp
@@ -796,6 +796,28 @@ static void processTileSizesFromOpenMPConstruct(
}
}
+static pft::Evaluation *getNestedDoConstruct(pft::Evaluation &eval) {
+ for (pft::Evaluation &nested : eval.getNestedEvaluations()) {
+ // In an OpenMPConstruct there can be compiler directives:
+ // 1 <<OpenMPConstruct>>
+ // 2 CompilerDirective: !unroll
+ // <<DoConstruct>> -> 8
+ if (nested.getIf<parser::CompilerDirective>())
+ continue;
+ // Within a DoConstruct, there can be compiler directives, plus
+ // there is a DoStmt before the body:
+ // <<DoConstruct>> -> 8
+ // 3 NonLabelDoStmt -> 7: do i = 1, n
+ // <<DoConstruct>> -> 7
+ if (nested.getIf<parser::NonLabelDoStmt>())
+ continue;
+ assert(nested.getIf<parser::DoConstruct>() &&
+ "Unexpected construct in the nested evaluations");
+ return &nested;
+ }
+ llvm_unreachable("Expected do loop to be in the nested evaluations");
+}
+
/// Populates the sizes vector with values if the given OpenMPConstruct
/// contains a loop construct with an inner tiling construct.
void collectTileSizesFromOpenMPConstruct(
@@ -818,7 +840,7 @@ int64_t collectLoopRelatedInfo(
int64_t numCollapse = 1;
// Collect the loops to collapse.
- lower::pft::Evaluation *doConstructEval = &eval.getFirstNestedEvaluation();
+ lower::pft::Evaluation *doConstructEval = getNestedDoConstruct(eval);
if (doConstructEval->getIf<parser::DoConstruct>()->IsDoConcurrent()) {
TODO(currentLocation, "Do Concurrent in Worksharing loop construct");
}
@@ -844,7 +866,8 @@ void collectLoopRelatedInfo(
fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
// Collect the loops to collapse.
- lower::pft::Evaluation *doConstructEval = &eval.getFirstNestedEvaluation();
+ lower::pft::Evaluation *doConstructEval = getNestedDoConstruct(eval);
+ assert(doConstructEval && "Expected do loop to be in the nested evaluation");
if (doConstructEval->getIf<parser::DoConstruct>()->IsDoConcurrent()) {
TODO(currentLocation, "Do Concurrent in Worksharing loop construct");
}
@@ -885,9 +908,8 @@ void collectLoopRelatedInfo(
iv.push_back(bounds->name.thing.symbol);
loopVarTypeSize = std::max(loopVarTypeSize,
bounds->name.thing.symbol->GetUltimate().size());
- collapseValue--;
- doConstructEval =
- &*std::next(doConstructEval->getNestedEvaluations().begin());
+ if (--collapseValue)
+ doConstructEval = getNestedDoConstruct(*doConstructEval);
} while (collapseValue > 0);
convertLoopBounds(converter, currentLocation, result, loopVarTypeSize);
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index 6d83ee62c3b73..ef237a01d0f7a 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -267,7 +267,7 @@ void OmpStructureChecker::CheckNestedBlock(const parser::OpenMPLoopConstruct &x,
for (auto &stmt : body) {
if (auto *dir{parser::Unwrap<parser::CompilerDirective>(stmt)}) {
context_.Say(dir->source,
- "Compiler directives are not allowed inside OpenMP loop constructs"_err_en_US);
+ "Compiler directives are not allowed inside OpenMP loop constructs"_warn_en_US);
} else if (parser::Unwrap<parser::DoConstruct>(stmt)) {
++nestedCount;
} else if (auto *omp{parser::Unwrap<parser::OpenMPLoopConstruct>(stmt)}) {
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 673ea4ef10e08..b992a4125ffcb 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2419,13 +2419,6 @@ void OmpAttributeVisitor::PrivatizeAssociatedLoopIndexAndCheckLoopLevel(
}
}
CheckAssocLoopLevel(level, GetAssociatedClause());
- } else {
- unsigned version{context_.langOptions().OpenMPVersion};
- context_.Say(GetContext().directiveSource,
- "A DO loop must follow the %s directive"_err_en_US,
- parser::ToUpperCaseLetters(
- llvm::omp::getOpenMPDirectiveName(GetContext().directive, version)
- .str()));
}
}
}
diff --git a/flang/test/Semantics/OpenMP/compiler-directives-loop.f90 b/flang/test/Semantics/OpenMP/compiler-directives-loop.f90
new file mode 100644
index 0000000000000..48b9529c95dc2
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/compiler-directives-loop.f90
@@ -0,0 +1,21 @@
+!RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=60 %s -o - | FileCheck %s
+
+! Check that this compiles successfully, but not rely on any specific output.
+
+!CHECK: omp.parallel
+
+program omp_cdir_crash
+ implicit none
+ integer, parameter :: n = 10
+ real :: a(n)
+ integer :: i
+
+!$omp parallel do
+!dir$ unroll
+ do i = 1, n
+ a(i) = real(i)
+ end do
+!$omp end parallel do
+
+ print *, 'a(1)=', a(1), ' a(n)=', a(n)
+end program omp_cdir_crash
diff --git a/flang/test/Semantics/OpenMP/loop-association.f90 b/flang/test/Semantics/OpenMP/loop-association.f90
index 9c79a91429fdf..4e63cafb3fda1 100644
--- a/flang/test/Semantics/OpenMP/loop-association.f90
+++ b/flang/test/Semantics/OpenMP/loop-association.f90
@@ -33,10 +33,9 @@
END DO outer
! Accept directives between parallel do and actual loop.
- !ERROR: A DO loop must follow the PARALLEL DO directive
!$OMP PARALLEL DO
!WARNING: Unrecognized compiler directive was ignored [-Wignored-directive]
- !ERROR: Compiler directives are not allowed inside OpenMP loop constructs
+ !WARNING: Compiler directives are not allowed inside OpenMP loop constructs
!DIR$ VECTOR ALIGNED
DO 20 i=1,N
a = a + 0.5
|
Comment on lines
+799
to
+819
| static pft::Evaluation *getNestedDoConstruct(pft::Evaluation &eval) { | ||
| for (pft::Evaluation &nested : eval.getNestedEvaluations()) { | ||
| // In an OpenMPConstruct there can be compiler directives: | ||
| // 1 <<OpenMPConstruct>> | ||
| // 2 CompilerDirective: !unroll | ||
| // <<DoConstruct>> -> 8 | ||
| if (nested.getIf<parser::CompilerDirective>()) | ||
| continue; | ||
| // Within a DoConstruct, there can be compiler directives, plus | ||
| // there is a DoStmt before the body: | ||
| // <<DoConstruct>> -> 8 | ||
| // 3 NonLabelDoStmt -> 7: do i = 1, n | ||
| // <<DoConstruct>> -> 7 | ||
| if (nested.getIf<parser::NonLabelDoStmt>()) | ||
| continue; | ||
| assert(nested.getIf<parser::DoConstruct>() && | ||
| "Unexpected construct in the nested evaluations"); | ||
| return &nested; | ||
| } | ||
| llvm_unreachable("Expected do loop to be in the nested evaluations"); | ||
| } |
Contributor
There was a problem hiding this comment.
Will this work with loop generating OpenMP constructs?
!$omp parallel do
!$omp tile sizes(4)
do i = 1, n
a(i) = real(i)
end do
!$omp end parallel do
Contributor
There was a problem hiding this comment.
If getNestedDoConstruct will only be used in restricted cases where we might have to skip directives (and there is no chance of loop generating constructs appearing) then may be we should rename it.
Contributor
Author
There was a problem hiding this comment.
We crash on this testcase (we crashed on it before my loop nest parser work). The lowering code does not support that at the moment.
Contributor
Author
There was a problem hiding this comment.
getNestedDoConstruct is currently used in all cases where we traverse the nest, but it has the same limitations as the preexisting code.
| // <<DoConstruct>> -> 8 | ||
| if (nested.getIf<parser::CompilerDirective>()) | ||
| continue; | ||
| // Within a DoConstruct, there can be compiler directives, plus |
Contributor
There was a problem hiding this comment.
Is there a typo here?
Contributor
Author
There was a problem hiding this comment.
I can't see it... 😐 Is there something strange about the wording?
Contributor
Author
There was a problem hiding this comment.
Ok, now I see an improper use of a comma.
Contributor
There was a problem hiding this comment.
Sorry Kryztof, I was wondering whether you were mentioning the compiler directives point twice (once above as part of the skipping the compiler directive and also here).
Contributor
Author
There was a problem hiding this comment.
Yes, the directives could appear in both constructs. I wanted to mention it because the prior code assumed that DoStmt would be the second nested evaluation.
aadeshps-mcw
pushed a commit
to aadeshps-mcw/llvm-project
that referenced
this pull request
Nov 26, 2025
…#169346) PR168884 flagged compiler directives (!dir$ ...) inside OpenMP loop constructs as errors. This caused some customer applications to fail to compile (issue 169229). Downgrade the error to a warning, and gracefully ignore compiler directives when lowering loop constructs to MLIR. Fixes llvm#169229
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.
PR168884 flagged compiler directives (!dir$ ...) inside OpenMP loop constructs as errors. This caused some customer applications to fail to compile (issue 169229).
Downgrade the error to a warning, and gracefully ignore compiler directives when lowering loop constructs to MLIR.
Fixes #169229