From 463b1490b64884715c9c263122c04af1915c972e Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Tue, 11 Nov 2025 11:53:06 -0600 Subject: [PATCH 1/4] [OpenMP] Add more comments to `ConstructDecompositionT.h`, NFC --- .../Frontend/OpenMP/ConstructDecompositionT.h | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h index d702273cec9ec..3918cecfc1e65 100644 --- a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h +++ b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h @@ -68,17 +68,16 @@ find_unique(Container &&container, Predicate &&pred) { namespace tomp { -// ClauseType - Either instance of ClauseT, or a type derived from ClauseT. -// -// This is the clause representation in the code using this infrastructure. -// -// HelperType - A class that implements two member functions: +// ClauseType: Either an instance of ClauseT, or a type derived from ClauseT. +// This is the clause representation in the code using this infrastructure. // +// HelperType: A class that implements two member functions: // // Return the base object of the given object, if any. // std::optional getBaseObject(const Object &object) const // // Return the iteration variable of the outermost loop associated // // with the construct being worked on, if any. // std::optional getLoopIterVar() const + template struct ConstructDecompositionT { using ClauseTy = ClauseType; @@ -181,27 +180,32 @@ struct ConstructDecompositionT { std::enable_if_t::UnionTrait::value, void> addClauseSymsToMap(U &&item, const ClauseTy *); - // Apply a clause to the only directive that allows it. If there are no + // Apply the clause to the only directive that allows it. If there are no // directives that allow it, or if there is more that one, do not apply // anything and return false, otherwise return true. bool applyToUnique(const ClauseTy *node); - // Apply a clause to the first directive in given range that allows it. + // Apply the clause to the first directive in given range that allows it. // If such a directive does not exist, return false, otherwise return true. template bool applyToFirst(const ClauseTy *node, llvm::iterator_range range); - // Apply a clause to the innermost directive that allows it. If such a + // Apply the clause to the innermost directive that allows it. If such a // directive does not exist, return false, otherwise return true. bool applyToInnermost(const ClauseTy *node); - // Apply a clause to the outermost directive that allows it. If such a + // Apply the clause to the outermost directive that allows it. If such a // directive does not exist, return false, otherwise return true. bool applyToOutermost(const ClauseTy *node); + // Apply the clause to all directives that allow it, and which satisfy + // the predicate: bool shouldApply(LeafReprInternal). If no such + // directives exist, return false, otherwise return true. template bool applyIf(const ClauseTy *node, Predicate shouldApply); + // Apply the clause to all directives that allow it. If no such directives + // exist, return false, otherwise return true. bool applyToAll(const ClauseTy *node); template @@ -983,7 +987,7 @@ bool ConstructDecompositionT::applyClause( return dir == llvm::omp::Directive::OMPD_simd || llvm::is_contained(getWorksharingLoop(), dir); case ReductionModifier::Task: - if (alreadyApplied) + if (alreadyApplied) // Not an error return false; // According to [5.2:135:16-18], "task" only applies to "parallel" and // worksharing constructs. From be3b2d7ec5a1feb01f1f9a0a64a2449986f4d2b6 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Tue, 11 Nov 2025 11:55:10 -0600 Subject: [PATCH 2/4] [OpenMP] Apply COLLAPSE to innermost leaf that allows it 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. --- .../llvm/Frontend/OpenMP/ConstructDecompositionT.h | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h index 3918cecfc1e65..c8eebbf42a68e 100644 --- a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h +++ b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h @@ -501,18 +501,7 @@ template bool ConstructDecompositionT::applyClause( const tomp::clause::CollapseT &clause, const ClauseTy *node) { - // Apply "collapse" to the innermost directive. If it's not one that - // allows it flag an error. - if (!leafs.empty()) { - auto &last = leafs.back(); - - if (llvm::omp::isAllowedClauseForDirective(last.id, node->id, version)) { - last.clauses.push_back(node); - return true; - } - } - - return false; + return applyToInnermost(node); } // DEFAULT From 6b95c8ffbe67d0a2021f9da06df79cef72bc1f32 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Tue, 4 Nov 2025 15:03:33 -0600 Subject: [PATCH 3/4] [OpenMP] Report errors when construct decomposition fails 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. --- .../Frontend/OpenMP/ConstructDecompositionT.h | 104 +++++++++++------ .../Frontend/OpenMPDecompositionTest.cpp | 105 +++++++++++++++++- 2 files changed, 173 insertions(+), 36 deletions(-) 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> output; + llvm::SmallVector> 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 clauses; @@ -456,10 +469,9 @@ bool ConstructDecompositionT::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::applyClause( }); }); - return applied; + if (!applied) + return error(node, ErrorCode::NoLeafPrivatizing); + return true; } // COLLAPSE @@ -501,7 +515,9 @@ template bool ConstructDecompositionT::applyClause( const tomp::clause::CollapseT &clause, const ClauseTy *node) { - return applyToInnermost(node); + if (!applyToInnermost(node)) + return error(node, ErrorCode::NoLeafAllowing); + return true; } // DEFAULT @@ -516,7 +532,9 @@ bool ConstructDecompositionT::applyClause( const tomp::clause::DefaultT &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::applyClause( applied = true; } - return applied; + if (!applied) + return error(node, ErrorCode::NoLeafAllowing); + return true; } // IF @@ -679,10 +699,12 @@ bool ConstructDecompositionT::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 bool ConstructDecompositionT::applyClause( const tomp::clause::LastprivateT &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::applyClause( llvm::omp::Clause::OMPC_shared, tomp::clause::SharedT{/*List=*/sharedObjects}); dirParallel->clauses.push_back(shared); - applied = true; } // [5.2:340:24] @@ -748,7 +766,6 @@ bool ConstructDecompositionT::applyClause( llvm::omp::Clause::OMPC_shared, tomp::clause::SharedT{/*List=*/sharedObjects}); dirTeams->clauses.push_back(shared); - applied = true; } } @@ -772,11 +789,10 @@ bool ConstructDecompositionT::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::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 bool ConstructDecompositionT::applyClause( const tomp::clause::NowaitT &clause, const ClauseTy *node) { - return applyToOutermost(node); + if (!applyToOutermost(node)) + return error(node, ErrorCode::NoLeafAllowing); + return true; } // OMPX_ATTRIBUTE @@ -855,8 +873,9 @@ template bool ConstructDecompositionT::applyClause( const tomp::clause::OmpxAttributeT &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 bool ConstructDecompositionT::applyClause( const tomp::clause::OmpxBareT &clause, const ClauseTy *node) { - return applyToOutermost(node); + if (!applyToOutermost(node)) + return error(node, ErrorCode::NoLeafAllowing); + return true; } // ORDER @@ -879,7 +900,9 @@ bool ConstructDecompositionT::applyClause( const tomp::clause::OrderT &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 bool ConstructDecompositionT::applyClause( const tomp::clause::PrivateT &clause, const ClauseTy *node) { - return applyToInnermost(node); + if (!applyToInnermost(node)) + return error(node, ErrorCode::NoLeafAllowing); + return true; } // REDUCTION @@ -996,31 +1021,37 @@ bool ConstructDecompositionT::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 sharedObjects; llvm::transform(objects, std::back_inserter(sharedObjects), @@ -1067,11 +1098,10 @@ bool ConstructDecompositionT::applyClause( /*LocatorList=*/std::move(tofrom)}}); dirTarget->clauses.push_back(map); - applied = true; } } - return applied; + return true; } // SHARED @@ -1086,7 +1116,9 @@ bool ConstructDecompositionT::applyClause( const tomp::clause::SharedT &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::applyClause( const tomp::clause::ThreadLimitT &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 &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 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 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 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 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 From a5ac7161c69e99d7090ea43207e86eafb92fb868 Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Tue, 11 Nov 2025 12:55:10 -0600 Subject: [PATCH 4/4] format --- .../llvm/Frontend/OpenMP/ConstructDecompositionT.h | 8 ++++---- llvm/unittests/Frontend/OpenMPDecompositionTest.cpp | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h index c4d0d58d3bb76..36b49e69650d8 100644 --- a/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h +++ b/llvm/include/llvm/Frontend/OpenMP/ConstructDecompositionT.h @@ -69,10 +69,10 @@ 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 + 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. diff --git a/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp b/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp index 123c8252ab1e3..23c3c4d5d192c 100644 --- a/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp +++ b/llvm/unittests/Frontend/OpenMPDecompositionTest.cpp @@ -277,7 +277,7 @@ struct StringifyClause { std::string Str; }; -std::string stringify(const omp::Clause &C) { +std::string stringify(const omp::Clause &C) { // return StringifyClause(C).Str; } @@ -308,8 +308,8 @@ std::string stringify(tomp::ErrorCode E) { std::string stringify(std::pair &ER) { std::stringstream Stream; - Stream << "error while applying '" << stringify(*ER.first) << "': " - << stringify(ER.second); + Stream << "error while applying '" << stringify(*ER.first) + << "': " << stringify(ER.second); return Stream.str(); }