[OpenMP] Report errors when construct decomposition fails #167568
Merged
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
As per the wording from 5.2, the COLLAPSE clause applies once to the entire construct. The 6.0 spec has a somewhat similar wording with the same intent. In practice, apply the clause to the innermost leaf constituent that allows it, without requiring it to be the exact innermost leaf.
Store the list of errors in the ConsstructDecomposition class in addition to the broken up output. This not used in flang yet, because the splitting happens at a time when diagnostic messages can no longer be emitted. Use unit tests to test this instead.
Member
|
@llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesStore the list of errors in the ConsstructDecomposition class in addition to the broken up output. This not used in flang yet, because the splitting happens at a time when diagnostic messages can no longer be emitted. Use unit tests to test this instead. Full diff: https://github.com/llvm/llvm-project/pull/167568.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h
index c8eebbf42a68e..c4d0d58d3bb76 100644
--- a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h
+++ b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h
@@ -68,6 +68,13 @@ find_unique(Container &&container, Predicate &&pred) {
namespace tomp {
+enum struct ErrorCode : int {
+ NoLeafAllowing, // No leaf that allows this clause
+ NoLeafPrivatizing, // No leaf that has a privatizing clause
+ InvalidDirNameMod, // Invalid directive name modifier
+ RedModNotApplied, // Reduction modifier not applied
+};
+
// ClauseType: Either an instance of ClauseT, or a type derived from ClauseT.
// This is the clause representation in the code using this infrastructure.
//
@@ -114,10 +121,16 @@ struct ConstructDecompositionT {
}
tomp::ListT<DirectiveWithClauses<ClauseType>> output;
+ llvm::SmallVector<std::pair<const ClauseType *, ErrorCode>> errors;
private:
bool split();
+ bool error(const ClauseTy *node, ErrorCode ec) {
+ errors.emplace_back(node, ec);
+ return false;
+ }
+
struct LeafReprInternal {
llvm::omp::Directive id = llvm::omp::Directive::OMPD_unknown;
tomp::type::ListT<const ClauseTy *> clauses;
@@ -456,10 +469,9 @@ bool ConstructDecompositionT<C, H>::applyClause(Specific &&specific,
// S Some clauses are permitted only on a single leaf construct of the
// S combined or composite construct, in which case the effect is as if
// S the clause is applied to that specific construct. (p339, 31-33)
- if (applyToUnique(node))
- return true;
-
- return false;
+ if (!applyToUnique(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// --- Specific clauses -----------------------------------------------
@@ -487,7 +499,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
});
});
- return applied;
+ if (!applied)
+ return error(node, ErrorCode::NoLeafPrivatizing);
+ return true;
}
// COLLAPSE
@@ -501,7 +515,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::CollapseT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- return applyToInnermost(node);
+ if (!applyToInnermost(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// DEFAULT
@@ -516,7 +532,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::DefaultT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
// [5.2:340:31]
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// FIRSTPRIVATE
@@ -644,7 +662,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
applied = true;
}
- return applied;
+ if (!applied)
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// IF
@@ -679,10 +699,12 @@ bool ConstructDecompositionT<C, H>::applyClause(
hasDir->clauses.push_back(unmodified);
return true;
}
- return false;
+ return error(node, ErrorCode::InvalidDirNameMod);
}
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// LASTPRIVATE
@@ -708,12 +730,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::LastprivateT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- bool applied = false;
-
// [5.2:340:21]
- applied = applyToAll(node);
- if (!applied)
- return false;
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
auto inFirstprivate = [&](const ObjectTy &object) {
if (ClauseSet *set = findClausesWith(object)) {
@@ -739,7 +758,6 @@ bool ConstructDecompositionT<C, H>::applyClause(
llvm::omp::Clause::OMPC_shared,
tomp::clause::SharedT<TypeTy, IdTy, ExprTy>{/*List=*/sharedObjects});
dirParallel->clauses.push_back(shared);
- applied = true;
}
// [5.2:340:24]
@@ -748,7 +766,6 @@ bool ConstructDecompositionT<C, H>::applyClause(
llvm::omp::Clause::OMPC_shared,
tomp::clause::SharedT<TypeTy, IdTy, ExprTy>{/*List=*/sharedObjects});
dirTeams->clauses.push_back(shared);
- applied = true;
}
}
@@ -772,11 +789,10 @@ bool ConstructDecompositionT<C, H>::applyClause(
/*Mapper=*/std::nullopt, /*Iterator=*/std::nullopt,
/*LocatorList=*/std::move(tofrom)}});
dirTarget->clauses.push_back(map);
- applied = true;
}
}
- return applied;
+ return true;
}
// LINEAR
@@ -802,7 +818,7 @@ bool ConstructDecompositionT<C, H>::applyClause(
const ClauseTy *node) {
// [5.2:341:15.1]
if (!applyToInnermost(node))
- return false;
+ return error(node, ErrorCode::NoLeafAllowing);
// [5.2:341:15.2], [5.2:341:19]
auto dirSimd = findDirective(llvm::omp::Directive::OMPD_simd);
@@ -847,7 +863,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::NowaitT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- return applyToOutermost(node);
+ if (!applyToOutermost(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// OMPX_ATTRIBUTE
@@ -855,8 +873,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::OmpxAttributeT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- // ERROR: no leaf that allows clause
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// OMPX_BARE
@@ -864,7 +883,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::OmpxBareT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- return applyToOutermost(node);
+ if (!applyToOutermost(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// ORDER
@@ -879,7 +900,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::OrderT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
// [5.2:340:31]
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// PRIVATE
@@ -894,7 +917,9 @@ template <typename C, typename H>
bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::PrivateT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
- return applyToInnermost(node);
+ if (!applyToInnermost(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// REDUCTION
@@ -996,31 +1021,37 @@ bool ConstructDecompositionT<C, H>::applyClause(
/*List=*/objects}});
ReductionModifier effective = modifier.value_or(ReductionModifier::Default);
- bool effectiveApplied = false;
+ bool modifierApplied = false;
+ bool allowingLeaf = false;
// Walk over the leaf constructs starting from the innermost, and apply
// the clause as required by the spec.
for (auto &leaf : llvm::reverse(leafs)) {
if (!llvm::omp::isAllowedClauseForDirective(leaf.id, node->id, version))
continue;
+ // Found a leaf that allows this clause. Keep track of this for better
+ // error reporting.
+ allowingLeaf = true;
if (!applyToParallel && &leaf == dirParallel)
continue;
if (!applyToTeams && &leaf == dirTeams)
continue;
// Some form of the clause will be applied past this point.
- if (isValidModifier(leaf.id, effective, effectiveApplied)) {
+ if (isValidModifier(leaf.id, effective, modifierApplied)) {
// Apply clause with modifier.
leaf.clauses.push_back(node);
- effectiveApplied = true;
+ modifierApplied = true;
} else {
// Apply clause without modifier.
leaf.clauses.push_back(unmodified);
}
// The modifier must be applied to some construct.
- applied = effectiveApplied;
+ applied = modifierApplied;
}
+ if (!allowingLeaf)
+ return error(node, ErrorCode::NoLeafAllowing);
if (!applied)
- return false;
+ return error(node, ErrorCode::RedModNotApplied);
tomp::ObjectListT<IdTy, ExprTy> sharedObjects;
llvm::transform(objects, std::back_inserter(sharedObjects),
@@ -1067,11 +1098,10 @@ bool ConstructDecompositionT<C, H>::applyClause(
/*LocatorList=*/std::move(tofrom)}});
dirTarget->clauses.push_back(map);
- applied = true;
}
}
- return applied;
+ return true;
}
// SHARED
@@ -1086,7 +1116,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::SharedT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
// [5.2:340:31]
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// THREAD_LIMIT
@@ -1101,7 +1133,9 @@ bool ConstructDecompositionT<C, H>::applyClause(
const tomp::clause::ThreadLimitT<TypeTy, IdTy, ExprTy> &clause,
const ClauseTy *node) {
// [5.2:340:31]
- return applyToAll(node);
+ if (!applyToAll(node))
+ return error(node, ErrorCode::NoLeafAllowing);
+ return true;
}
// --- Splitting ------------------------------------------------------
diff --git a/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp b/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp
index e0c6b3904310c..123c8252ab1e3 100644
--- a/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp
@@ -277,16 +277,42 @@ struct StringifyClause {
std::string Str;
};
+std::string stringify(const omp::Clause &C) {
+ return StringifyClause(C).Str;
+}
+
std::string stringify(const omp::DirectiveWithClauses &DWC) {
std::stringstream Stream;
Stream << getOpenMPDirectiveName(DWC.id, llvm::omp::FallbackVersion).str();
for (const omp::Clause &C : DWC.clauses)
- Stream << ' ' << StringifyClause(C).Str;
+ Stream << ' ' << stringify(C);
return Stream.str();
}
+std::string stringify(tomp::ErrorCode E) {
+ switch (E) {
+ case tomp::ErrorCode::NoLeafAllowing:
+ return "no leaf that allows this clause";
+ case tomp::ErrorCode::NoLeafPrivatizing:
+ return "no leaf with a privatizing clause";
+ case tomp::ErrorCode::InvalidDirNameMod:
+ return "invalid directive name modifier";
+ case tomp::ErrorCode::RedModNotApplied:
+ return "the reduction modifier cannot be applied";
+ }
+ return "unrecognized error code " + std::to_string(llvm::to_underlying(E));
+}
+
+std::string stringify(std::pair<const omp::Clause *, tomp::ErrorCode> &ER) {
+ std::stringstream Stream;
+
+ Stream << "error while applying '" << stringify(*ER.first) << "': "
+ << stringify(ER.second);
+ return Stream.str();
+}
+
// --- Tests ----------------------------------------------------------
namespace red {
@@ -1109,4 +1135,81 @@ TEST_F(OpenMPDecompositionTest, Misc1) {
std::string Dir0 = stringify(Dec.output[0]);
ASSERT_EQ(Dir0, "simd linear(, , (x)) lastprivate(, (x))");
}
+
+// --- Failure/error reporting tests
+
+TEST_F(OpenMPDecompositionTest, Error1) {
+ // "parallel for at(compilation)" is invalid because the "at" clause
+ // does not apply to either "parallel" or "for".
+
+ omp::List<omp::Clause> Clauses{
+ {OMPC_at, omp::clause::At{omp::clause::At::ActionTime::Compilation}},
+ };
+
+ omp::ConstructDecomposition Dec(AnyVersion, Helper, OMPD_parallel_for,
+ Clauses);
+ ASSERT_EQ(Dec.errors.size(), 1u);
+ std::string Err0 = stringify(Dec.errors[0]);
+ ASSERT_EQ(Err0,
+ "error while applying 'at(0)': no leaf that allows this clause");
+}
+
+TEST_F(OpenMPDecompositionTest, Error2) {
+ // "parallel loop allocate(x) private(x)" is invalid because "allocate"
+ // can only be applied to "parallel", while "private" is applied to "loop".
+ // This violates the requirement that the leaf with an "allocate" also has
+ // a privatizing clause.
+
+ omp::Object x{"x"};
+
+ omp::List<omp::Clause> Clauses{
+ {OMPC_allocate, omp::clause::Allocate{{std::nullopt, std::nullopt, {x}}}},
+ {OMPC_private, omp::clause::Private{{x}}},
+ };
+
+ omp::ConstructDecomposition Dec(AnyVersion, Helper, OMPD_parallel_loop,
+ Clauses);
+ ASSERT_EQ(Dec.errors.size(), 1u);
+ std::string Err0 = stringify(Dec.errors[0]);
+ ASSERT_EQ(Err0, "error while applying 'allocate(, , (x))': no leaf with a "
+ "privatizing clause");
+}
+
+TEST_F(OpenMPDecompositionTest, Error3) {
+ // "parallel for if(target: e)" is invalid because the "target" directive-
+ // name-modifier does not refer to a constituent directive.
+
+ omp::ExprTy e;
+
+ omp::List<omp::Clause> Clauses{
+ {OMPC_if, omp::clause::If{{llvm::omp::Directive::OMPD_target, e}}},
+ };
+
+ omp::ConstructDecomposition Dec(AnyVersion, Helper, OMPD_parallel_for,
+ Clauses);
+ ASSERT_EQ(Dec.errors.size(), 1u);
+ std::string Err0 = stringify(Dec.errors[0]);
+ ASSERT_EQ(Err0, "error while applying 'if(target, expr)': invalid directive "
+ "name modifier");
+}
+
+TEST_F(OpenMPDecompositionTest, Error4) {
+ // "masked taskloop reduction(+, task: x)" is invalid because the "task"
+ // modifier can only be applied to "parallel" or a worksharing directive.
+
+ omp::Object x{"x"};
+ auto Add = red::makeOp(omp::clause::DefinedOperator::IntrinsicOperator::Add);
+ auto TaskMod = omp::clause::Reduction::ReductionModifier::Task;
+
+ omp::List<omp::Clause> Clauses{
+ {OMPC_reduction, omp::clause::Reduction{{TaskMod, {Add}, {x}}}},
+ };
+
+ omp::ConstructDecomposition Dec(AnyVersion, Helper, OMPD_masked_taskloop,
+ Clauses);
+ ASSERT_EQ(Dec.errors.size(), 1u);
+ std::string Err0 = stringify(Dec.errors[0]);
+ ASSERT_EQ(Err0, "error while applying 'reduction(2, (3), (x))': the "
+ "reduction modifier cannot be applied");
+}
} // namespace
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
git-crd
pushed a commit
to git-crd/crd-llvm-project
that referenced
this pull request
Nov 13, 2025
Store the list of errors in the ConsstructDecomposition class in addition to the broken up output. This not used in flang yet, because the splitting happens at a time when diagnostic messages can no longer be emitted. Use unit tests to test this instead.
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.
Store the list of errors in the ConsstructDecomposition class in addition to the broken up output.
This not used in flang yet, because the splitting happens at a time when diagnostic messages can no longer be emitted. Use unit tests to test this instead.