llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Zahira Ammarguellat (zahiraam) <details> <summary>Changes</summary> This patch 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". --- Patch is 357.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/214717.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 (+127-34) - (modified) clang/test/OpenMP/fuse_codegen.cpp (+228-66) - (modified) clang/test/OpenMP/interchange_codegen.cpp (+722-624) - (added) clang/test/OpenMP/loop_transform_iv.c (+129) - (modified) clang/test/OpenMP/reverse_codegen.cpp (+402-329) - (modified) clang/test/OpenMP/stripe_codegen.cpp (+176-110) - (modified) clang/test/OpenMP/tile_codegen.cpp (+176-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..12f10c806107a 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -8018,6 +8018,8 @@ struct LoopIterationSpace final { /// check that the number of iterations for this particular counter must be /// finished. Expr *FinalCondition = nullptr; + /// True if this iteration space corresponds to a range-based for loop. + bool IsRangeFor = false; }; /// Scan an AST subtree, checking that no decls in the CollapsedLoopVarDecls @@ -9692,6 +9694,7 @@ static bool checkOpenMPIterationSpace( ResultIterSpaces[CurrentNestedLoopCount].Subtract = ISC.shouldSubtractStep(); ResultIterSpaces[CurrentNestedLoopCount].IsStrictCompare = ISC.isStrictTestOp(); + ResultIterSpaces[CurrentNestedLoopCount].IsRangeFor = (CXXFor != nullptr); std::tie(ResultIterSpaces[CurrentNestedLoopCount].MinValue, ResultIterSpaces[CurrentNestedLoopCount].MaxValue) = ISC.buildMinMaxValues(DSA.getCurScope(), Captures); @@ -9971,6 +9974,25 @@ static Stmt *buildPreInits(ASTContext &Context, ArrayRef<Stmt *> PreInits) { return CompoundStmt::Create(Context, PreInits, FPOptionsOverride(), {}, {}); } +/// Helper to determine if a loop should skip finalization. +/// Returns true for range-based for loops or loops with non-arithmetic +/// counters. +static bool shouldSkipLoopFinalization(Stmt *LoopStmt) { + if (isa<CXXForRangeStmt>(LoopStmt)) + return true; + + auto *For = dyn_cast<ForStmt>(LoopStmt); + if (!For) + return false; + + auto *InitDecl = dyn_cast_or_null<DeclStmt>(For->getInit()); + if (!InitDecl || !InitDecl->isSingleDecl()) + return false; + + auto *InitVar = dyn_cast<VarDecl>(InitDecl->getSingleDecl()); + return InitVar && !InitVar->getType()->isArithmeticType(); +} + /// Build postupdate expression for the given list of postupdates expressions. static Expr *buildPostUpdate(Sema &S, ArrayRef<Expr *> PostUpdates) { Expr *PostUpdate = nullptr; @@ -10672,16 +10694,38 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr, } // Build final: IS.CounterVar = IS.Start + IS.NumIters * IS.Step - ExprResult Final = - buildCounterUpdate(SemaRef, CurScope, UpdLoc, CounterVar, - ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/214717 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
