Skip to content

Commit 8c3fbaf

Browse files
authored
[Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (#140532)
This patch is closely related to #139293 and addresses an existing issue in the loop transformation codebase. Specifically, it corrects the handling of the `NumGeneratedLoops` variable in `OMPLoopTransformationDirective` AST nodes and its inheritors (such as OMPUnrollDirective, OMPTileDirective, etc.). Previously, this variable was inaccurately set for certain transformations like reverse or tile. While this did not lead to functional bugs, since the value was only checked to determine whether it was greater than zero or equal to zero, the inconsistency could introduce problems when supporting more complex directives in the future.
1 parent 17f5b8b commit 8c3fbaf

File tree

4 files changed

+26
-17
lines changed

4 files changed

+26
-17
lines changed

clang/include/clang/AST/StmtOpenMP.h

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5787,10 +5787,13 @@ class OMPReverseDirective final : public OMPLoopTransformationDirective {
57875787
TransformedStmtOffset,
57885788
};
57895789

5790-
explicit OMPReverseDirective(SourceLocation StartLoc, SourceLocation EndLoc)
5790+
explicit OMPReverseDirective(SourceLocation StartLoc, SourceLocation EndLoc,
5791+
unsigned NumLoops)
57915792
: OMPLoopTransformationDirective(OMPReverseDirectiveClass,
57925793
llvm::omp::OMPD_reverse, StartLoc,
5793-
EndLoc, 1) {}
5794+
EndLoc, NumLoops) {
5795+
setNumGeneratedLoops(NumLoops);
5796+
}
57945797

57955798
void setPreInits(Stmt *PreInits) {
57965799
Data->getChildren()[PreInitsOffset] = PreInits;
@@ -5806,19 +5809,23 @@ class OMPReverseDirective final : public OMPLoopTransformationDirective {
58065809
/// \param C Context of the AST.
58075810
/// \param StartLoc Location of the introducer (e.g. the 'omp' token).
58085811
/// \param EndLoc Location of the directive's end (e.g. the tok::eod).
5812+
/// \param NumLoops Number of affected loops
58095813
/// \param AssociatedStmt The outermost associated loop.
58105814
/// \param TransformedStmt The loop nest after tiling, or nullptr in
58115815
/// dependent contexts.
58125816
/// \param PreInits Helper preinits statements for the loop nest.
5813-
static OMPReverseDirective *
5814-
Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc,
5815-
Stmt *AssociatedStmt, Stmt *TransformedStmt, Stmt *PreInits);
5817+
static OMPReverseDirective *Create(const ASTContext &C,
5818+
SourceLocation StartLoc,
5819+
SourceLocation EndLoc,
5820+
Stmt *AssociatedStmt, unsigned NumLoops,
5821+
Stmt *TransformedStmt, Stmt *PreInits);
58165822

58175823
/// Build an empty '#pragma omp reverse' AST node for deserialization.
58185824
///
58195825
/// \param C Context of the AST.
5820-
/// \param NumClauses Number of clauses to allocate.
5821-
static OMPReverseDirective *CreateEmpty(const ASTContext &C);
5826+
/// \param NumLoops Number of associated loops to allocate
5827+
static OMPReverseDirective *CreateEmpty(const ASTContext &C,
5828+
unsigned NumLoops);
58225829

58235830
/// Gets/sets the associated loops after the transformation, i.e. after
58245831
/// de-sugaring.
@@ -5857,7 +5864,7 @@ class OMPInterchangeDirective final : public OMPLoopTransformationDirective {
58575864
: OMPLoopTransformationDirective(OMPInterchangeDirectiveClass,
58585865
llvm::omp::OMPD_interchange, StartLoc,
58595866
EndLoc, NumLoops) {
5860-
setNumGeneratedLoops(3 * NumLoops);
5867+
setNumGeneratedLoops(NumLoops);
58615868
}
58625869

58635870
void setPreInits(Stmt *PreInits) {

clang/lib/AST/StmtOpenMP.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -471,18 +471,21 @@ OMPUnrollDirective *OMPUnrollDirective::CreateEmpty(const ASTContext &C,
471471
OMPReverseDirective *
472472
OMPReverseDirective::Create(const ASTContext &C, SourceLocation StartLoc,
473473
SourceLocation EndLoc, Stmt *AssociatedStmt,
474-
Stmt *TransformedStmt, Stmt *PreInits) {
474+
unsigned NumLoops, Stmt *TransformedStmt,
475+
Stmt *PreInits) {
475476
OMPReverseDirective *Dir = createDirective<OMPReverseDirective>(
476-
C, {}, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc);
477+
C, {}, AssociatedStmt, TransformedStmtOffset + 1, StartLoc, EndLoc,
478+
NumLoops);
477479
Dir->setTransformedStmt(TransformedStmt);
478480
Dir->setPreInits(PreInits);
479481
return Dir;
480482
}
481483

482-
OMPReverseDirective *OMPReverseDirective::CreateEmpty(const ASTContext &C) {
484+
OMPReverseDirective *OMPReverseDirective::CreateEmpty(const ASTContext &C,
485+
unsigned NumLoops) {
483486
return createEmptyDirective<OMPReverseDirective>(
484487
C, /*NumClauses=*/0, /*HasAssociatedStmt=*/true,
485-
TransformedStmtOffset + 1, SourceLocation(), SourceLocation());
488+
TransformedStmtOffset + 1, SourceLocation(), SourceLocation(), NumLoops);
486489
}
487490

488491
OMPInterchangeDirective *OMPInterchangeDirective::Create(

clang/lib/Sema/SemaOpenMP.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15140,7 +15140,7 @@ StmtResult SemaOpenMP::ActOnOpenMPReverseDirective(Stmt *AStmt,
1514015140
// instantiated.
1514115141
if (SemaRef.CurContext->isDependentContext())
1514215142
return OMPReverseDirective::Create(Context, StartLoc, EndLoc, AStmt,
15143-
nullptr, nullptr);
15143+
NumLoops, nullptr, nullptr);
1514415144

1514515145
assert(LoopHelpers.size() == NumLoops &&
1514615146
"Expecting a single-dimensional loop iteration space");
@@ -15299,7 +15299,7 @@ StmtResult SemaOpenMP::ActOnOpenMPReverseDirective(Stmt *AStmt,
1529915299
ForStmt(Context, Init.get(), Cond.get(), nullptr, Incr.get(),
1530015300
ReversedBody, LoopHelper.Init->getBeginLoc(),
1530115301
LoopHelper.Init->getBeginLoc(), LoopHelper.Inc->getEndLoc());
15302-
return OMPReverseDirective::Create(Context, StartLoc, EndLoc, AStmt,
15302+
return OMPReverseDirective::Create(Context, StartLoc, EndLoc, AStmt, NumLoops,
1530315303
ReversedFor,
1530415304
buildPreInits(Context, PreInits));
1530515305
}

clang/lib/Serialization/ASTReaderStmt.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3602,11 +3602,10 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) {
36023602
}
36033603

36043604
case STMT_OMP_REVERSE_DIRECTIVE: {
3605-
assert(Record[ASTStmtReader::NumStmtFields] == 1 &&
3606-
"Reverse directive accepts only a single loop");
3605+
unsigned NumLoops = Record[ASTStmtReader::NumStmtFields];
36073606
assert(Record[ASTStmtReader::NumStmtFields + 1] == 0 &&
36083607
"Reverse directive has no clauses");
3609-
S = OMPReverseDirective::CreateEmpty(Context);
3608+
S = OMPReverseDirective::CreateEmpty(Context, NumLoops);
36103609
break;
36113610
}
36123611

0 commit comments

Comments
 (0)