llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Zahira Ammarguellat (zahiraam) <details> <summary>Changes</summary> This PR relands #<!-- -->208533 which was reverted in #<!-- -->212832 due to test failures with iterator-based loops. Original Change: #<!-- -->208533 fixes loop variable finalization for OpenMP 6.0 loop-transformations constructs: tile, stripe, reverse, interchange and fuse to comply with spec requirement page 371, lines 19-21. The spec requires that "After the execution of the loop-transforming construct, the loop-iteration variables of any of its transformation-affected loops have the values that they would have without the loop-transforming directive". What's Fixed in This Reland: The original implementation attempted to finalize all loop variables, including iterators in range-based for loops (CXXForRangeStmt). This caused issues because the finalization formula `final_value = lower_bound + num_iterations * step` only applies to arithmetic types (integers, floats). --- Patch is 218.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/212853.diff 10 Files Affected: - (modified) clang/include/clang/AST/StmtOpenMP.h (+51-16) - (modified) clang/lib/AST/StmtOpenMP.cpp (+24-24) - (modified) clang/lib/CodeGen/CGStmtOpenMP.cpp (+25) - (modified) clang/lib/Sema/SemaOpenMP.cpp (+86-25) - (modified) clang/test/OpenMP/fuse_codegen.cpp (+216-66) - (modified) clang/test/OpenMP/interchange_codegen.cpp (+192-98) - (added) clang/test/OpenMP/loop_transform_final_iv.c (+129) - (modified) clang/test/OpenMP/reverse_codegen.cpp (+154-85) - (modified) clang/test/OpenMP/stripe_codegen.cpp (+172-110) - (modified) clang/test/OpenMP/tile_codegen.cpp (+172-110) ``````````diff diff --git a/clang/include/clang/AST/StmtOpenMP.h b/clang/include/clang/AST/StmtOpenMP.h index dbc76e7df8ecd..e668e214ddcb2 100644 --- a/clang/include/clang/AST/StmtOpenMP.h +++ b/clang/include/clang/AST/StmtOpenMP.h @@ -5601,6 +5601,7 @@ class OMPTileDirective final enum { PreInitsOffset = 0, TransformedStmtOffset, + FinalsOffset, }; explicit OMPTileDirective(SourceLocation StartLoc, SourceLocation EndLoc, @@ -5617,6 +5618,8 @@ class OMPTileDirective final Data->getChildren()[TransformedStmtOffset] = S; } + void setFinals(Stmt *Finals) { Data->getChildren()[FinalsOffset] = Finals; } + public: /// Create a new AST node representation for '#pragma omp tile'. /// @@ -5630,11 +5633,11 @@ class OMPTileDirective final /// \param TransformedStmt The loop nest after tiling, or nullptr in /// dependent contexts. /// \param PreInits Helper preinits statements for the loop nest. - static OMPTileDirective *Create(const ASTContext &C, SourceLocation StartLoc, - SourceLocation EndLoc, - ArrayRef<OMPClause *> Clauses, - unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits); + /// \param Finals Loop variable finalization statements. + static OMPTileDirective * + Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, + ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals); /// Build an empty '#pragma omp tile' AST node for deserialization. /// @@ -5662,6 +5665,9 @@ class OMPTileDirective final /// Return preinits statement. Stmt *getPreInits() const { return Data->getChildren()[PreInitsOffset]; } + /// Return finals statement (loop variable finalization). + Stmt *getFinals() const { return Data->getChildren()[FinalsOffset]; } + static bool classof(const Stmt *T) { return T->getStmtClass() == OMPTileDirectiveClass; } @@ -5677,6 +5683,7 @@ class OMPStripeDirective final enum { PreInitsOffset = 0, TransformedStmtOffset, + FinalsOffset, }; explicit OMPStripeDirective(SourceLocation StartLoc, SourceLocation EndLoc, @@ -5693,6 +5700,8 @@ class OMPStripeDirective final Data->getChildren()[TransformedStmtOffset] = S; } + void setFinals(Stmt *Finals) { Data->getChildren()[FinalsOffset] = Finals; } + public: /// Create a new AST node representation for '#pragma omp stripe'. /// @@ -5706,10 +5715,11 @@ class OMPStripeDirective final /// \param TransformedStmt The loop nest after striping, or nullptr in /// dependent contexts. /// \param PreInits Helper preinits statements for the loop nest. + /// \param Finals Loop variable finalization statements. static OMPStripeDirective * Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits); + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals); /// Build an empty '#pragma omp stripe' AST node for deserialization. /// @@ -5736,6 +5746,9 @@ class OMPStripeDirective final /// Return preinits statement. Stmt *getPreInits() const { return Data->getChildren()[PreInitsOffset]; } + /// Return finals statement (loop variable finalization). + Stmt *getFinals() const { return Data->getChildren()[FinalsOffset]; } + static bool classof(const Stmt *T) { return T->getStmtClass() == OMPStripeDirectiveClass; } @@ -5832,6 +5845,7 @@ class OMPReverseDirective final enum { PreInitsOffset = 0, TransformedStmtOffset, + FinalsOffset, }; explicit OMPReverseDirective(SourceLocation StartLoc, SourceLocation EndLoc, @@ -5848,6 +5862,8 @@ class OMPReverseDirective final Data->getChildren()[TransformedStmtOffset] = S; } + void setFinals(Stmt *Finals) { Data->getChildren()[FinalsOffset] = Finals; } + public: /// Create a new AST node representation for '#pragma omp reverse'. /// @@ -5859,11 +5875,11 @@ class OMPReverseDirective final /// \param TransformedStmt The loop nest after tiling, or nullptr in /// dependent contexts. /// \param PreInits Helper preinits statements for the loop nest. - static OMPReverseDirective *Create(const ASTContext &C, - SourceLocation StartLoc, - SourceLocation EndLoc, - Stmt *AssociatedStmt, unsigned NumLoops, - Stmt *TransformedStmt, Stmt *PreInits); + /// \param Finals Loop variable finalization statements. + static OMPReverseDirective * + Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, + Stmt *AssociatedStmt, unsigned NumLoops, Stmt *TransformedStmt, + Stmt *PreInits, Stmt *Finals); /// Build an empty '#pragma omp reverse' AST node for deserialization. /// @@ -5881,6 +5897,9 @@ class OMPReverseDirective final /// Return preinits statement. Stmt *getPreInits() const { return Data->getChildren()[PreInitsOffset]; } + /// Return finals statement. + Stmt *getFinals() const { return Data->getChildren()[FinalsOffset]; } + static bool classof(const Stmt *T) { return T->getStmtClass() == OMPReverseDirectiveClass; } @@ -5903,6 +5922,7 @@ class OMPInterchangeDirective final enum { PreInitsOffset = 0, TransformedStmtOffset, + FinalsOffset, }; explicit OMPInterchangeDirective(SourceLocation StartLoc, @@ -5919,6 +5939,8 @@ class OMPInterchangeDirective final Data->getChildren()[TransformedStmtOffset] = S; } + void setFinals(Stmt *Finals) { Data->getChildren()[FinalsOffset] = Finals; } + public: /// Create a new AST node representation for '#pragma omp interchange'. /// @@ -5932,10 +5954,11 @@ class OMPInterchangeDirective final /// \param TransformedStmt The loop nest after tiling, or nullptr in /// dependent contexts. /// \param PreInits Helper preinits statements for the loop nest. + /// \param Finals Loop variable finalization statements. static OMPInterchangeDirective * Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits); + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals); /// Build an empty '#pragma omp interchange' AST node for deserialization. /// @@ -5954,6 +5977,9 @@ class OMPInterchangeDirective final /// Return preinits statement. Stmt *getPreInits() const { return Data->getChildren()[PreInitsOffset]; } + /// Return finals statement. + Stmt *getFinals() const { return Data->getChildren()[FinalsOffset]; } + static bool classof(const Stmt *T) { return T->getStmtClass() == OMPInterchangeDirectiveClass; } @@ -6009,6 +6035,7 @@ class OMPFuseDirective final enum { PreInitsOffset = 0, TransformedStmtOffset, + FinalsOffset, }; explicit OMPFuseDirective(SourceLocation StartLoc, SourceLocation EndLoc) @@ -6023,6 +6050,8 @@ class OMPFuseDirective final Data->getChildren()[TransformedStmtOffset] = S; } + void setFinals(Stmt *Finals) { Data->getChildren()[FinalsOffset] = Finals; } + public: /// Create a new AST node representation for #pragma omp fuse' /// @@ -6038,10 +6067,13 @@ class OMPFuseDirective final /// \param TransformedStmt The loop nest after fusion, or nullptr in /// dependent /// \param PreInits Helper preinits statements for the loop nest - static OMPFuseDirective * - Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, - ArrayRef<OMPClause *> Clauses, unsigned NumGeneratedTopLevelLoops, - Stmt *AssociatedStmt, Stmt *TransformedStmt, Stmt *PreInits); + /// \param Finals Loop variable finalization statements + static OMPFuseDirective *Create(const ASTContext &C, SourceLocation StartLoc, + SourceLocation EndLoc, + ArrayRef<OMPClause *> Clauses, + unsigned NumGeneratedTopLevelLoops, + Stmt *AssociatedStmt, Stmt *TransformedStmt, + Stmt *PreInits, Stmt *Finals); /// Build an empty '#pragma omp fuse' AST node for deserialization /// @@ -6060,6 +6092,9 @@ class OMPFuseDirective final /// Return preinits statement. Stmt *getPreInits() const { return Data->getChildren()[PreInitsOffset]; } + /// Return finals statement. + Stmt *getFinals() const { return Data->getChildren()[FinalsOffset]; } + static bool classof(const Stmt *T) { return T->getStmtClass() == OMPFuseDirectiveClass; } diff --git a/clang/lib/AST/StmtOpenMP.cpp b/clang/lib/AST/StmtOpenMP.cpp index 9d6b315effb41..13f1d2d94aa85 100644 --- a/clang/lib/AST/StmtOpenMP.cpp +++ b/clang/lib/AST/StmtOpenMP.cpp @@ -452,12 +452,12 @@ OMPTileDirective * OMPTileDirective::Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits) { + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals) { OMPTileDirective *Dir = createDirective<OMPTileDirective>( - C, Clauses, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc, - NumLoops); + C, Clauses, AssociatedStmt, FinalsOffset + 1, StartLoc, EndLoc, NumLoops); Dir->setTransformedStmt(TransformedStmt); Dir->setPreInits(PreInits); + Dir->setFinals(Finals); return Dir; } @@ -465,20 +465,19 @@ OMPTileDirective *OMPTileDirective::CreateEmpty(const ASTContext &C, unsigned NumClauses, unsigned NumLoops) { return createEmptyDirective<OMPTileDirective>( - C, NumClauses, /*HasAssociatedStmt=*/true, TransformedStmtOffset + 1, + C, NumClauses, /*HasAssociatedStmt=*/true, FinalsOffset + 1, SourceLocation(), SourceLocation(), NumLoops); } -OMPStripeDirective * -OMPStripeDirective::Create(const ASTContext &C, SourceLocation StartLoc, - SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, - unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits) { +OMPStripeDirective *OMPStripeDirective::Create( + const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, + ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals) { OMPStripeDirective *Dir = createDirective<OMPStripeDirective>( - C, Clauses, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc, - NumLoops); + C, Clauses, AssociatedStmt, FinalsOffset + 1, StartLoc, EndLoc, NumLoops); Dir->setTransformedStmt(TransformedStmt); Dir->setPreInits(PreInits); + Dir->setFinals(Finals); return Dir; } @@ -486,7 +485,7 @@ OMPStripeDirective *OMPStripeDirective::CreateEmpty(const ASTContext &C, unsigned NumClauses, unsigned NumLoops) { return createEmptyDirective<OMPStripeDirective>( - C, NumClauses, /*HasAssociatedStmt=*/true, TransformedStmtOffset + 1, + C, NumClauses, /*HasAssociatedStmt=*/true, FinalsOffset + 1, SourceLocation(), SourceLocation(), NumLoops); } @@ -516,31 +515,31 @@ OMPReverseDirective * OMPReverseDirective::Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, Stmt *AssociatedStmt, unsigned NumLoops, Stmt *TransformedStmt, - Stmt *PreInits) { + Stmt *PreInits, Stmt *Finals) { OMPReverseDirective *Dir = createDirective<OMPReverseDirective>( - C, {}, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc, - NumLoops); + C, {}, AssociatedStmt, FinalsOffset + 1, StartLoc, EndLoc, NumLoops); Dir->setTransformedStmt(TransformedStmt); Dir->setPreInits(PreInits); + Dir->setFinals(Finals); return Dir; } OMPReverseDirective *OMPReverseDirective::CreateEmpty(const ASTContext &C, unsigned NumLoops) { return createEmptyDirective<OMPReverseDirective>( - C, /*NumClauses=*/0, /*HasAssociatedStmt=*/true, - TransformedStmtOffset + 1, SourceLocation(), SourceLocation(), NumLoops); + C, /*NumClauses=*/0, /*HasAssociatedStmt=*/true, FinalsOffset + 1, + SourceLocation(), SourceLocation(), NumLoops); } OMPInterchangeDirective *OMPInterchangeDirective::Create( const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, unsigned NumLoops, Stmt *AssociatedStmt, - Stmt *TransformedStmt, Stmt *PreInits) { + Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals) { OMPInterchangeDirective *Dir = createDirective<OMPInterchangeDirective>( - C, Clauses, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc, - NumLoops); + C, Clauses, AssociatedStmt, FinalsOffset + 1, StartLoc, EndLoc, NumLoops); Dir->setTransformedStmt(TransformedStmt); Dir->setPreInits(PreInits); + Dir->setFinals(Finals); return Dir; } @@ -548,7 +547,7 @@ OMPInterchangeDirective * OMPInterchangeDirective::CreateEmpty(const ASTContext &C, unsigned NumClauses, unsigned NumLoops) { return createEmptyDirective<OMPInterchangeDirective>( - C, NumClauses, /*HasAssociatedStmt=*/true, TransformedStmtOffset + 1, + C, NumClauses, /*HasAssociatedStmt=*/true, FinalsOffset + 1, SourceLocation(), SourceLocation(), NumLoops); } @@ -576,12 +575,13 @@ OMPSplitDirective *OMPSplitDirective::CreateEmpty(const ASTContext &C, OMPFuseDirective *OMPFuseDirective::Create( const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc, ArrayRef<OMPClause *> Clauses, unsigned NumGeneratedTopLevelLoops, - Stmt *AssociatedStmt, Stmt *TransformedStmt, Stmt *PreInits) { + Stmt *AssociatedStmt, Stmt *TransformedStmt, Stmt *PreInits, Stmt *Finals) { OMPFuseDirective *Dir = createDirective<OMPFuseDirective>( - C, Clauses, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc); + C, Clauses, AssociatedStmt, FinalsOffset + 1, StartLoc, EndLoc); Dir->setTransformedStmt(TransformedStmt); Dir->setPreInits(PreInits); + Dir->setFinals(Finals); Dir->setNumGeneratedTopLevelLoops(NumGeneratedTopLevelLoops); return Dir; } @@ -589,7 +589,7 @@ OMPFuseDirective *OMPFuseDirective::Create( OMPFuseDirective *OMPFuseDirective::CreateEmpty(const ASTContext &C, unsigned NumClauses) { OMPFuseDirective *Dir = createEmptyDirective<OMPFuseDirective>( - C, NumClauses, /*HasAssociatedStmt=*/true, TransformedStmtOffset + 1, + C, NumClauses, /*HasAssociatedStmt=*/true, FinalsOffset + 1, SourceLocation(), SourceLocation()); return Dir; } diff --git a/clang/lib/CodeGen/CGStmtOpenMP.cpp b/clang/lib/CodeGen/CGStmtOpenMP.cpp index 97c43b4c0b384..ebf67f2fdf0a1 100644 --- a/clang/lib/CodeGen/CGStmtOpenMP.cpp +++ b/clang/lib/CodeGen/CGStmtOpenMP.cpp @@ -3221,18 +3221,33 @@ void CodeGenFunction::EmitOMPTileDirective(const OMPTileDirective &S) { // Emit the de-sugared statement. OMPTransformDirectiveScopeRAII TileScope(*this, &S); EmitStmt(S.getTransformedStmt()); + + // Emit loop variable finalization as required by OpenMP 6.0 spec to restore + // original loop variable values after the loop-transformation construct. + if (auto *Finals = S.getFinals()) + EmitStmt(Finals); } void CodeGenFunction::EmitOMPStripeDirective(const OMPStripeDirective &S) { // Emit the de-sugared statement. OMPTransformDirectiveScopeRAII StripeScope(*this, &S); EmitStmt(S.getTransformedStmt()); + + // Emit loop variable finalization as required by OpenMP 6.0 spec to restore + // original loop variable values after the loop-transformation construct. + if (auto *Finals = S.getFinals()) + EmitStmt(Finals); } void CodeGenFunction::EmitOMPReverseDirective(const OMPReverseDirective &S) { // Emit the de-sugared statement. OMPTransformDirectiveScopeRAII ReverseScope(*this, &S); EmitStmt(S.getTransformedStmt()); + + // Emit loop variable finalization as required by OpenMP 6.0 spec to restore + // original loop variable values after the loop-transformation construct. + if (auto *Finals = S.getFinals()) + EmitStmt(Finals); } void CodeGenFunction::EmitOMPSplitDirective(const OMPSplitDirective &S) { @@ -3246,12 +3261,22 @@ void CodeGenFunction::EmitOMPInterchangeDirective( // Emit the de-sugared statement. OMPTransformDirectiveScopeRAII InterchangeScope(*this, &S); EmitStmt(S.getTransformedStmt()); + + // Emit loop variable finalization as required by OpenMP 6.0 spec to restore + // original loop variable values after the loop-transformation construct. + if (auto *Finals = S.getFinals()) + EmitStmt(Finals); } void CodeGenFunction::EmitOMPFuseDirective(const OMPFuseDirective &S) { // Emit the de-sugared statement OMPTransformDirectiveScopeRAII FuseScope(*this, &S); EmitStmt(S.getTransformedStmt()); + + // Emit loop variable finalization as required by OpenMP 6.0 spec to restore + // original loop variable values after the loop-transformation construct. + if (auto *Finals = S.getFinals()) + EmitStmt(Finals); } void CodeGenFunction::EmitOMPUnrollDirective(const OMPUnrollDirective &S) { diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 76bb0d38d428f..95938ecef1279 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -15033,6 +15033,48 @@ static Expr *makeFloorIVRef(Sema &SemaRef, ArrayRef<VarDecl *> FloorIndVars, OrigCntVar->getExprLoc()); } +/// Build loop variable finalization statement from HelperExprs.Finals. +/// Returns a CompoundStmt containing all finalization statements, or nullptr +/// if there are no finalization statements. +/// Note: Loops with non-arithmetic loop variables (e.g., iterators) are skipped +/// because finalization only applies to integer/floating-point counters. +static Stmt * +buildLoopFinalization(ASTContext &Context, + ArrayRef<OMPLoopBasedDirective::HelperExprs> LoopHelpers, + ArrayRef<Stmt *> LoopStmts) { + assert(LoopHelpers.size() == LoopStmts.size() && + "LoopHelpers and LoopStmts must have the same size"); + SmallVector<Stmt *, 8> FinalizationStmts; + for (const auto &[Helper, LoopStmt] : llvm::zip(LoopHelpers, LoopStmts)) { + // Skip finalization for range-based for loops (CXXForRangeStmt) since + // their loop variables are iterators, not integer counters. + if (isa<CXXForRangeStmt>(LoopStmt)) + continue; + + // For ForStmt, check if the loop variable is arithmetic. + if (auto *For = dyn_cast<ForStmt>(LoopStmt)) { + if (auto *InitDeclStmt = dyn_cast_or_null<DeclStmt>(For->getInit())) { + if (InitDeclStmt->isSingleDecl()) { + if (auto *InitVar = + dyn_cast<VarDecl>(InitDeclStmt->getSingleDecl())) { + // Skip finalization if loop variable is not arithmetic. + if (!InitVar->getType()->isArithmeticType()) + continue; + } + } + } + } + + for (auto *Final : Helper.Finals) + if (Final) + FinalizationStmts.push_back(Final); + } + if (FinalizationStmts.empty()) + return nullptr; + return CompoundStmt::Create(Context, FinalizationStmts, FPOptionsOverride(), + SourceLocation(), SourceLocation()); +} + StmtResult SemaOpenMP::ActOnOpenMPTileDirective(ArrayRef<OMPClause *> Clauses, ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/212853 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
