[flang-commits] [flang] [flang][OpenMP] Store bad ExecutionPartConstruct in LoopSequence (PR #187556)
Krzysztof Parzyszek via flang-commits
flang-commits at lists.llvm.org
Fri Mar 20 09:45:45 PDT 2026
https://github.com/kparzysz updated https://github.com/llvm/llvm-project/pull/187556
>From 7d3379ca600347797a945feb6270963def5d5a5d Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Tue, 17 Mar 2026 13:08:46 -0500
Subject: [PATCH 1/3] [flang][OpenMP] Allow "Reason" messages to not have
source locations
When explanatory messages are generated there may be cases when there
is no satisfactory source location to apply them to. This patch allows
storing such messages without a source location.
The messages will be equipped with a source location at the time when
they are attached to the main error message (usually it will be the
same location as used for the main message).
Issue: https://github.com/llvm/llvm-project/issues/185287
---
flang/include/flang/Semantics/openmp-utils.h | 17 ++++++++++++++---
flang/lib/Semantics/check-omp-loop.cpp | 18 +++++++++---------
flang/lib/Semantics/openmp-utils.cpp | 16 ++++++++++++++--
3 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/flang/include/flang/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h
index d7f46cbc7bd62..66bbbeb4ee0e7 100644
--- a/flang/include/flang/Semantics/openmp-utils.h
+++ b/flang/include/flang/Semantics/openmp-utils.h
@@ -23,6 +23,7 @@
#include "flang/Semantics/tools.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
#include <optional>
#include <string>
@@ -116,12 +117,22 @@ MaybeExpr MakeEvaluateExpr(const parser::OmpStylizedInstance &inp);
struct Reason {
parser::Messages msgs;
- template <typename... Ts> Reason &Say(Ts &&...args) {
- msgs.Say(std::forward<Ts>(args)...);
+ // Allow messages without a source location. They will acquire a location
+ // during AttachTo.
+ template <typename... Ts>
+ Reason &Say(parser::CharBlock source, Ts &&...args) {
+ auto &msg{msgs.Say(source, std::forward<Ts>(args)...)};
+ if (source.empty()) {
+ unsourced_.insert(&msg);
+ }
return *this;
}
operator bool() const { return !msgs.empty(); }
- parser::Message &AttachTo(parser::Message &msg);
+ parser::Message &AttachTo(parser::CharBlock source, parser::Message &msg);
+
+private:
+ // Set of messages without a source location.
+ llvm::DenseSet<const parser::Message *> unsourced_;
};
std::pair<std::optional<int64_t>, Reason> GetArgumentValueWithReason(
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index 8f360daf4cfdc..f09b5f315dcc2 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -247,6 +247,7 @@ void OmpStructureChecker::CheckNestedConstruct(
const parser::OmpDirectiveSpecification &beginSpec{x.BeginDir()};
llvm::omp::Directive dir{beginSpec.DirId()};
unsigned version{context_.langOptions().OpenMPVersion};
+ parser::CharBlock beginSource{beginSpec.DirName().source};
// End-directive is not allowed in such cases:
// do 100 i = ...
@@ -257,7 +258,6 @@ void OmpStructureChecker::CheckNestedConstruct(
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 share a loop-terminating statement"_err_en_US,
@@ -297,12 +297,12 @@ void OmpStructureChecker::CheckNestedConstruct(
if (std::optional<int64_t> numLoops{sequence.length()}) {
if (*numLoops == 0) {
- context_.Say(beginSpec.DirName().source,
+ context_.Say(beginSource,
"This construct should contain a DO-loop or a loop-nest-generating OpenMP construct"_err_en_US);
} else {
auto assoc{llvm::omp::getDirectiveAssociation(dir)};
if (*numLoops > 1 && assoc == llvm::omp::Association::LoopNest) {
- context_.Say(beginSpec.DirName().source,
+ context_.Say(beginSource,
"This construct applies to a loop nest, but has a loop sequence of "
"length %" PRId64 ""_err_en_US,
*numLoops);
@@ -310,12 +310,12 @@ void OmpStructureChecker::CheckNestedConstruct(
if (assoc == llvm::omp::Association::LoopSeq) {
if (auto requiredCount{GetRequiredCount(needFirst, needCount)}) {
if (*requiredCount > 0 && *numLoops < *requiredCount) {
- auto &msg{context_.Say(beginSpec.DirName().source,
+ auto &msg{context_.Say(beginSource,
"This construct requires a sequence of %" PRId64
" loops, but the loop sequence has a length of %" PRId64
""_err_en_US,
*requiredCount, *numLoops)};
- rangeReason.AttachTo(msg);
+ rangeReason.AttachTo(beginSource, msg);
}
}
}
@@ -334,18 +334,18 @@ void OmpStructureChecker::CheckNestedConstruct(
if (needDepth && haveDepth && *haveDepth > 0) {
if (*needDepth > *haveDepth) {
if (needPerfect) {
- auto &msg{context_.Say(beginSpec.DirName().source,
+ auto &msg{context_.Say(beginSource,
"This construct requires a perfect nest of depth %" PRId64
", but the associated nest is a perfect nest of depth %" PRId64
""_err_en_US,
*needDepth, *haveDepth)};
- depthReason.AttachTo(msg);
+ depthReason.AttachTo(beginSource, msg);
} else {
- auto &msg{context_.Say(beginSpec.DirName().source,
+ auto &msg{context_.Say(beginSource,
"This construct requires a nest of depth %" PRId64
", but the associated nest has a depth of %" PRId64 ""_err_en_US,
*needDepth, *haveDepth)};
- depthReason.AttachTo(msg);
+ depthReason.AttachTo(beginSource, msg);
}
}
}
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index e104096cb3af1..116d04cf93968 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -532,8 +532,20 @@ MaybeExpr MakeEvaluateExpr(const parser::OmpStylizedInstance &inp) {
instance.u);
}
-parser::Message &Reason::AttachTo(parser::Message &msg) {
- msgs.AttachTo(msg);
+parser::Message &Reason::AttachTo(
+ parser::CharBlock source, parser::Message &msg) {
+ parser::Messages sourced;
+ for (auto &&msg : msgs.messages()) {
+ if (unsourced_.contains(&msg)) {
+ llvm::StringRef fmt{"%s"};
+ sourced.Say(source,
+ parser::MessageFixedText(fmt.data(), fmt.size(), msg.severity()),
+ msg.ToString());
+ } else {
+ sourced.Say(std::move(msg));
+ }
+ }
+ sourced.AttachTo(msg);
return msg;
}
>From c6a64975363612d6bebeb14e0dba32b90147c914 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Wed, 18 Mar 2026 09:45:43 -0500
Subject: [PATCH 2/3] [flang][OpenMP] Store bad ExecutionPartConstruct in
LoopSequence
LoopSequence keeps track of whether it contains code that would be an
invalid intervening code, or that would prevent loop nesting from
being a perfect nesting. To improve the quality of diagnostic messages
store the pointer to the offending parser::ExecutionPartConstruct.
Issue: https://github.com/llvm/llvm-project/issues/185287
---
flang/include/flang/Semantics/openmp-utils.h | 14 +--
flang/lib/Semantics/openmp-utils.cpp | 94 +++++++++++++++++---
2 files changed, 88 insertions(+), 20 deletions(-)
diff --git a/flang/include/flang/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h
index 66bbbeb4ee0e7..fdd45081e52b5 100644
--- a/flang/include/flang/Semantics/openmp-utils.h
+++ b/flang/include/flang/Semantics/openmp-utils.h
@@ -213,12 +213,14 @@ struct LoopSequence {
Depth calculateDepths() const;
Depth getNestedDepths() const;
- /// True if the sequence contains any code (besides transformable loops)
- /// that is not a valid intervening code.
- bool hasInvalidIC_{false};
- /// True if the sequence contains any code (besides transformable loops)
- /// that is not a valid transparent code.
- bool hasOpaqueIC_{false};
+ /// The construct that is not a loop or a loop-transforming construct,
+ /// that is also not a valid intervening code. Unset if no such code is
+ /// present.
+ const parser::ExecutionPartConstruct *invalidIC_{nullptr};
+ /// The construct that is not a loop or a loop-transforming construct,
+ /// whose presence would prevent perfect nesting of loops (i.e. code that
+ /// is not "transparent" to a perfect nest).
+ const parser::ExecutionPartConstruct *opaqueIC_{nullptr};
/// Precalculated length of the sequence. Note that this is different from
/// the number of children because a child may result in a sequence, for
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index 116d04cf93968..0fcc3e725307d 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -787,6 +787,9 @@ bool IsTransformableLoop(const parser::DoConstruct &loop) {
}
bool IsTransformableLoop(const parser::OpenMPLoopConstruct &omp) {
+ if (IsFullUnroll(omp)) {
+ return false;
+ }
return IsLoopTransforming(omp.BeginDir().DirId());
}
@@ -931,6 +934,60 @@ std::optional<int64_t> GetRequiredCount(
return std::nullopt;
}
+#ifdef EXPENSIVE_CHECKS
+namespace {
+/// Check that for every value x of type T, there will be a "source" member
+/// somewhere in x. This is to specifically make sure that parser::GetSource
+/// will return something for any parser::ExecutionPartConstruct.
+
+template <typename...> struct HasSourceT {
+ static constexpr bool value{false};
+};
+
+template <typename T> struct HasSourceT<T> {
+private:
+ using U = llvm::remove_cvref_t<T>;
+
+ static constexpr bool check() {
+ if constexpr (parser::HasSource<U>::value) {
+ return true;
+ } else if constexpr (ConstraintTrait<U>) {
+ return HasSourceT<decltype(U::thing)>::value;
+ } else if constexpr (WrapperTrait<U>) {
+ return HasSourceT<decltype(U::v)>::value;
+ } else if constexpr (TupleTrait<U>) {
+ return HasSourceT<decltype(U::t)>::value;
+ } else if constexpr (UnionTrait<U>) {
+ return HasSourceT<decltype(U::u)>::value;
+ } else {
+ return false;
+ }
+ }
+
+public:
+ static constexpr bool value{check()};
+};
+
+template <> struct HasSourceT<parser::ErrorRecovery> {
+ static constexpr bool value{true};
+};
+
+template <typename T> struct HasSourceT<common::Indirection<T>> {
+ static constexpr bool value{HasSourceT<T>::value};
+};
+
+template <typename... Ts> struct HasSourceT<std::tuple<Ts...>> {
+ static constexpr bool value{(HasSourceT<Ts>::value || ...)};
+};
+
+template <typename... Ts> struct HasSourceT<std::variant<Ts...>> {
+ static constexpr bool value{(HasSourceT<Ts>::value && ...)};
+};
+
+static_assert(HasSourceT<parser::ExecutionPartConstruct>::value);
+} // namespace
+#endif // EXPENSIVE_CHECKS
+
LoopSequence::LoopSequence(const parser::ExecutionPartConstruct &root,
unsigned version, bool allowAllLoops)
: version_(version), allowAllLoops_(allowAllLoops) {
@@ -957,10 +1014,11 @@ std::unique_ptr<LoopSequence::Construct> LoopSequence::createConstructEntry(
return std::make_unique<Construct>(body, &code);
}
} else if (auto *omp{parser::Unwrap<parser::OpenMPLoopConstruct>(code)}) {
- if (IsTransformableLoop(*omp)) {
- auto &body{std::get<parser::Block>(omp->t)};
- return std::make_unique<Construct>(body, &code);
- }
+ // Allow all loop constructs. This helps with better diagnostics, e.g.
+ // "this is not a loop-transforming construct", insted of just "this is
+ // not a valid intervening code".
+ auto &body{std::get<parser::Block>(omp->t)};
+ return std::make_unique<Construct>(body, &code);
}
return nullptr;
@@ -969,6 +1027,7 @@ std::unique_ptr<LoopSequence::Construct> LoopSequence::createConstructEntry(
void LoopSequence::createChildrenFromRange(
ExecutionPartIterator::IteratorType begin,
ExecutionPartIterator::IteratorType end) {
+ bool invalidWithEntry{false};
// Create children. If there is zero or one, this LoopSequence could be
// a nest. If there are more, it could be a proper sequence. In the latter
// case any code between consecutive children must be "transparent".
@@ -976,13 +1035,20 @@ void LoopSequence::createChildrenFromRange(
if (auto entry{createConstructEntry(code)}) {
children_.push_back(
LoopSequence(std::move(entry), version_, allowAllLoops_));
- if (!IsTransformableLoop(code)) {
- hasInvalidIC_ = true;
- hasOpaqueIC_ = true;
+ // Even when DO WHILE et al are allowed to have entries, still treat
+ // them as invalid intervening code.
+ // Give it priority over other kinds of invalid interveninig code.
+ if (!invalidWithEntry && !IsTransformableLoop(code)) {
+ invalidIC_ = &code;
+ invalidWithEntry = true;
}
} else {
- hasInvalidIC_ = hasInvalidIC_ || !IsValidInterveningCode(code);
- hasOpaqueIC_ = hasOpaqueIC_ || !IsTransparentInterveningCode(code);
+ if (!invalidIC_ && !IsValidInterveningCode(code)) {
+ invalidIC_ = &code;
+ }
+ if (!opaqueIC_ && !IsTransparentInterveningCode(code)) {
+ opaqueIC_ = &code;
+ }
}
}
}
@@ -1081,16 +1147,16 @@ LoopSequence::Depth LoopSequence::calculateDepths() const {
return Depth{0, 0};
}
- // Get the length of the nested sequence. The hasInvalidIC_ and hasOpaqueIC_
- // flags do not count canonical loop nests, but there can only be one for
- // depth to make sense.
+ // Get the length of the nested sequence. The invalidIC_ and opaqueIC_
+ // members do not count canonical loop nests, but there can only be one
+ // for depth to make sense.
std::optional<int64_t> length{getNestedLength()};
// Get the depths of the code nested in this sequence (e.g. contained in
// entry_), and use it as the basis for the depths of entry_->owner.
auto [semaDepth, perfDepth]{getNestedDepths()};
- if (hasInvalidIC_ || length.value_or(0) != 1) {
+ if (invalidIC_ || length.value_or(0) != 1) {
semaDepth = perfDepth = 0;
- } else if (hasOpaqueIC_ || length.value_or(0) != 1) {
+ } else if (opaqueIC_ || length.value_or(0) != 1) {
perfDepth = 0;
}
>From bae4197dbc6d7e8889a5d3fdddf25b29addf836f Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Fri, 20 Mar 2026 11:44:52 -0500
Subject: [PATCH 3/3] Revert "[flang][OpenMP] Allow "Reason" messages to not
have source locations"
This reverts commit 7d3379ca600347797a945feb6270963def5d5a5d.
---
flang/include/flang/Semantics/openmp-utils.h | 17 +++--------------
flang/lib/Semantics/check-omp-loop.cpp | 18 +++++++++---------
flang/lib/Semantics/openmp-utils.cpp | 16 ++--------------
3 files changed, 14 insertions(+), 37 deletions(-)
diff --git a/flang/include/flang/Semantics/openmp-utils.h b/flang/include/flang/Semantics/openmp-utils.h
index fdd45081e52b5..7b099d1214c83 100644
--- a/flang/include/flang/Semantics/openmp-utils.h
+++ b/flang/include/flang/Semantics/openmp-utils.h
@@ -23,7 +23,6 @@
#include "flang/Semantics/tools.h"
#include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/DenseSet.h"
#include <optional>
#include <string>
@@ -117,22 +116,12 @@ MaybeExpr MakeEvaluateExpr(const parser::OmpStylizedInstance &inp);
struct Reason {
parser::Messages msgs;
- // Allow messages without a source location. They will acquire a location
- // during AttachTo.
- template <typename... Ts>
- Reason &Say(parser::CharBlock source, Ts &&...args) {
- auto &msg{msgs.Say(source, std::forward<Ts>(args)...)};
- if (source.empty()) {
- unsourced_.insert(&msg);
- }
+ template <typename... Ts> Reason &Say(Ts &&...args) {
+ msgs.Say(std::forward<Ts>(args)...);
return *this;
}
operator bool() const { return !msgs.empty(); }
- parser::Message &AttachTo(parser::CharBlock source, parser::Message &msg);
-
-private:
- // Set of messages without a source location.
- llvm::DenseSet<const parser::Message *> unsourced_;
+ parser::Message &AttachTo(parser::Message &msg);
};
std::pair<std::optional<int64_t>, Reason> GetArgumentValueWithReason(
diff --git a/flang/lib/Semantics/check-omp-loop.cpp b/flang/lib/Semantics/check-omp-loop.cpp
index f09b5f315dcc2..8f360daf4cfdc 100644
--- a/flang/lib/Semantics/check-omp-loop.cpp
+++ b/flang/lib/Semantics/check-omp-loop.cpp
@@ -247,7 +247,6 @@ void OmpStructureChecker::CheckNestedConstruct(
const parser::OmpDirectiveSpecification &beginSpec{x.BeginDir()};
llvm::omp::Directive dir{beginSpec.DirId()};
unsigned version{context_.langOptions().OpenMPVersion};
- parser::CharBlock beginSource{beginSpec.DirName().source};
// End-directive is not allowed in such cases:
// do 100 i = ...
@@ -258,6 +257,7 @@ void OmpStructureChecker::CheckNestedConstruct(
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 share a loop-terminating statement"_err_en_US,
@@ -297,12 +297,12 @@ void OmpStructureChecker::CheckNestedConstruct(
if (std::optional<int64_t> numLoops{sequence.length()}) {
if (*numLoops == 0) {
- context_.Say(beginSource,
+ context_.Say(beginSpec.DirName().source,
"This construct should contain a DO-loop or a loop-nest-generating OpenMP construct"_err_en_US);
} else {
auto assoc{llvm::omp::getDirectiveAssociation(dir)};
if (*numLoops > 1 && assoc == llvm::omp::Association::LoopNest) {
- context_.Say(beginSource,
+ context_.Say(beginSpec.DirName().source,
"This construct applies to a loop nest, but has a loop sequence of "
"length %" PRId64 ""_err_en_US,
*numLoops);
@@ -310,12 +310,12 @@ void OmpStructureChecker::CheckNestedConstruct(
if (assoc == llvm::omp::Association::LoopSeq) {
if (auto requiredCount{GetRequiredCount(needFirst, needCount)}) {
if (*requiredCount > 0 && *numLoops < *requiredCount) {
- auto &msg{context_.Say(beginSource,
+ auto &msg{context_.Say(beginSpec.DirName().source,
"This construct requires a sequence of %" PRId64
" loops, but the loop sequence has a length of %" PRId64
""_err_en_US,
*requiredCount, *numLoops)};
- rangeReason.AttachTo(beginSource, msg);
+ rangeReason.AttachTo(msg);
}
}
}
@@ -334,18 +334,18 @@ void OmpStructureChecker::CheckNestedConstruct(
if (needDepth && haveDepth && *haveDepth > 0) {
if (*needDepth > *haveDepth) {
if (needPerfect) {
- auto &msg{context_.Say(beginSource,
+ auto &msg{context_.Say(beginSpec.DirName().source,
"This construct requires a perfect nest of depth %" PRId64
", but the associated nest is a perfect nest of depth %" PRId64
""_err_en_US,
*needDepth, *haveDepth)};
- depthReason.AttachTo(beginSource, msg);
+ depthReason.AttachTo(msg);
} else {
- auto &msg{context_.Say(beginSource,
+ auto &msg{context_.Say(beginSpec.DirName().source,
"This construct requires a nest of depth %" PRId64
", but the associated nest has a depth of %" PRId64 ""_err_en_US,
*needDepth, *haveDepth)};
- depthReason.AttachTo(beginSource, msg);
+ depthReason.AttachTo(msg);
}
}
}
diff --git a/flang/lib/Semantics/openmp-utils.cpp b/flang/lib/Semantics/openmp-utils.cpp
index 0fcc3e725307d..377f6623226bc 100644
--- a/flang/lib/Semantics/openmp-utils.cpp
+++ b/flang/lib/Semantics/openmp-utils.cpp
@@ -532,20 +532,8 @@ MaybeExpr MakeEvaluateExpr(const parser::OmpStylizedInstance &inp) {
instance.u);
}
-parser::Message &Reason::AttachTo(
- parser::CharBlock source, parser::Message &msg) {
- parser::Messages sourced;
- for (auto &&msg : msgs.messages()) {
- if (unsourced_.contains(&msg)) {
- llvm::StringRef fmt{"%s"};
- sourced.Say(source,
- parser::MessageFixedText(fmt.data(), fmt.size(), msg.severity()),
- msg.ToString());
- } else {
- sourced.Say(std::move(msg));
- }
- }
- sourced.AttachTo(msg);
+parser::Message &Reason::AttachTo(parser::Message &msg) {
+ msgs.AttachTo(msg);
return msg;
}
More information about the flang-commits
mailing list