From 463b1490b64884715c9c263122c04af1915c972e Mon Sep 17 00:00:00 2001 From: Krzysztof Parzyszek Date: Tue, 11 Nov 2025 11:53:06 -0600 Subject: [PATCH 1/2] [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/2] [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