Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][OpenMP] Diagnose badly-formed collapsed imperfect loop nests (#60678) #101305

Merged
merged 1 commit into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -11152,6 +11152,8 @@ def err_omp_loop_diff_cxx : Error<
"upper and lower loop bounds">;
def err_omp_loop_cannot_use_stmt : Error<
"'%0' statement cannot be used in OpenMP for loop">;
def err_omp_loop_bad_collapse_var : Error<
"cannot use variable %1 in collapsed imperfectly-nested loop %select{init|condition|increment}0 statement">;
def err_omp_simd_region_cannot_use_stmt : Error<
"'%0' statement cannot be used in OpenMP simd region">;
def warn_omp_loop_64_bit_var : Warning<
Expand Down
141 changes: 133 additions & 8 deletions clang/lib/Sema/SemaOpenMP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclOpenMP.h"
#include "clang/AST/OpenMPClause.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/StmtCXX.h"
#include "clang/AST/StmtOpenMP.h"
#include "clang/AST/StmtVisitor.h"
Expand Down Expand Up @@ -7668,6 +7669,52 @@ struct LoopIterationSpace final {
Expr *FinalCondition = nullptr;
};

/// Scan an AST subtree, checking that no decls in the CollapsedLoopVarDecls
/// set are referenced. Used for verifying loop nest structure before
/// performing a loop collapse operation.
class ForSubExprChecker final : public RecursiveASTVisitor<ForSubExprChecker> {
const llvm::SmallPtrSetImpl<const Decl *> &CollapsedLoopVarDecls;
VarDecl *ForbiddenVar = nullptr;
SourceRange ErrLoc;

public:
explicit ForSubExprChecker(
const llvm::SmallPtrSetImpl<const Decl *> &CollapsedLoopVarDecls)
: CollapsedLoopVarDecls(CollapsedLoopVarDecls) {}

// We want to visit implicit code, i.e. synthetic initialisation statements
// created during range-for lowering.
bool shouldVisitImplicitCode() const { return true; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused? Drop then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which bit do you think is unused? The next revision includes a comment on the shouldVisitImplicitCode definition (which is needed).


bool VisitDeclRefExpr(DeclRefExpr *E) {
ValueDecl *VD = E->getDecl();
if (!isa<VarDecl, BindingDecl>(VD))
return true;
VarDecl *V = VD->getPotentiallyDecomposedVarDecl();
if (V->getType()->isReferenceType()) {
VarDecl *VD = V->getDefinition();
if (VD->hasInit()) {
Expr *I = VD->getInit();
DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(I);
if (!DRE)
return true;
V = DRE->getDecl()->getPotentiallyDecomposedVarDecl();
}
}
Decl *Canon = V->getCanonicalDecl();
if (CollapsedLoopVarDecls.contains(Canon)) {
ForbiddenVar = V;
ErrLoc = E->getSourceRange();
return false;
}

return true;
}

VarDecl *getForbiddenVar() const { return ForbiddenVar; }
SourceRange getErrRange() const { return ErrLoc; }
};

/// Helper class for checking canonical form of the OpenMP loops and
/// extracting iteration space of each loop in the loop nest, that will be used
/// for IR generation.
Expand All @@ -7682,6 +7729,8 @@ class OpenMPIterationSpaceChecker {
SourceLocation DefaultLoc;
/// A location for diagnostics (when increment is not compatible).
SourceLocation ConditionLoc;
/// The set of variables declared within the (to be collapsed) loop nest.
const llvm::SmallPtrSetImpl<const Decl *> &CollapsedLoopVarDecls;
/// A source location for referring to loop init later.
SourceRange InitSrcRange;
/// A source location for referring to condition later.
Expand Down Expand Up @@ -7725,10 +7774,13 @@ class OpenMPIterationSpaceChecker {
Expr *Condition = nullptr;

public:
OpenMPIterationSpaceChecker(Sema &SemaRef, bool SupportsNonRectangular,
DSAStackTy &Stack, SourceLocation DefaultLoc)
OpenMPIterationSpaceChecker(
Sema &SemaRef, bool SupportsNonRectangular, DSAStackTy &Stack,
SourceLocation DefaultLoc,
const llvm::SmallPtrSetImpl<const Decl *> &CollapsedLoopDecls)
: SemaRef(SemaRef), SupportsNonRectangular(SupportsNonRectangular),
Stack(Stack), DefaultLoc(DefaultLoc), ConditionLoc(DefaultLoc) {}
Stack(Stack), DefaultLoc(DefaultLoc), ConditionLoc(DefaultLoc),
CollapsedLoopVarDecls(CollapsedLoopDecls) {}
/// Check init-expr for canonical loop form and save loop counter
/// variable - #Var and its initialization value - #LB.
bool checkAndSetInit(Stmt *S, bool EmitDiags = true);
Expand Down Expand Up @@ -8049,6 +8101,16 @@ bool OpenMPIterationSpaceChecker::checkAndSetInit(Stmt *S, bool EmitDiags) {
if (!ExprTemp->cleanupsHaveSideEffects())
S = ExprTemp->getSubExpr();

if (!CollapsedLoopVarDecls.empty()) {
ForSubExprChecker FSEC{CollapsedLoopVarDecls};
if (!FSEC.TraverseStmt(S)) {
SourceRange Range = FSEC.getErrRange();
SemaRef.Diag(Range.getBegin(), diag::err_omp_loop_bad_collapse_var)
<< Range.getEnd() << 0 << FSEC.getForbiddenVar();
return true;
}
}

InitSrcRange = S->getSourceRange();
if (Expr *E = dyn_cast<Expr>(S))
S = E->IgnoreParens();
Expand Down Expand Up @@ -8152,6 +8214,17 @@ bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
}
Condition = S;
S = getExprAsWritten(S);

if (!CollapsedLoopVarDecls.empty()) {
ForSubExprChecker FSEC{CollapsedLoopVarDecls};
if (!FSEC.TraverseStmt(S)) {
SourceRange Range = FSEC.getErrRange();
SemaRef.Diag(Range.getBegin(), diag::err_omp_loop_bad_collapse_var)
<< Range.getEnd() << 1 << FSEC.getForbiddenVar();
return true;
}
}

SourceLocation CondLoc = S->getBeginLoc();
auto &&CheckAndSetCond =
[this, IneqCondIsCanonical](BinaryOperatorKind Opcode, const Expr *LHS,
Expand Down Expand Up @@ -8250,6 +8323,16 @@ bool OpenMPIterationSpaceChecker::checkAndSetInc(Expr *S) {
if (!ExprTemp->cleanupsHaveSideEffects())
S = ExprTemp->getSubExpr();

if (!CollapsedLoopVarDecls.empty()) {
ForSubExprChecker FSEC{CollapsedLoopVarDecls};
if (!FSEC.TraverseStmt(S)) {
SourceRange Range = FSEC.getErrRange();
SemaRef.Diag(Range.getBegin(), diag::err_omp_loop_bad_collapse_var)
<< Range.getEnd() << 2 << FSEC.getForbiddenVar();
return true;
}
}

IncrementSrcRange = S->getSourceRange();
S = S->IgnoreParens();
if (auto *UO = dyn_cast<UnaryOperator>(S)) {
Expand Down Expand Up @@ -8971,8 +9054,9 @@ void SemaOpenMP::ActOnOpenMPLoopInitialization(SourceLocation ForLoc,
return;

DSAStack->loopStart();
llvm::SmallPtrSet<const Decl *, 1> EmptyDeclSet;
OpenMPIterationSpaceChecker ISC(SemaRef, /*SupportsNonRectangular=*/true,
*DSAStack, ForLoc);
*DSAStack, ForLoc, EmptyDeclSet);
if (!ISC.checkAndSetInit(Init, /*EmitDiags=*/false)) {
if (ValueDecl *D = ISC.getLoopDecl()) {
auto *VD = dyn_cast<VarDecl>(D);
Expand Down Expand Up @@ -9069,7 +9153,8 @@ static bool checkOpenMPIterationSpace(
Expr *OrderedLoopCountExpr,
SemaOpenMP::VarsWithInheritedDSAType &VarsWithImplicitDSA,
llvm::MutableArrayRef<LoopIterationSpace> ResultIterSpaces,
llvm::MapVector<const Expr *, DeclRefExpr *> &Captures) {
llvm::MapVector<const Expr *, DeclRefExpr *> &Captures,
const llvm::SmallPtrSetImpl<const Decl *> &CollapsedLoopVarDecls) {
bool SupportsNonRectangular = !isOpenMPLoopTransformationDirective(DKind);
// OpenMP [2.9.1, Canonical Loop Form]
// for (init-expr; test-expr; incr-expr) structured-block
Expand Down Expand Up @@ -9108,7 +9193,8 @@ static bool checkOpenMPIterationSpace(
return false;

OpenMPIterationSpaceChecker ISC(SemaRef, SupportsNonRectangular, DSA,
For ? For->getForLoc() : CXXFor->getForLoc());
For ? For->getForLoc() : CXXFor->getForLoc(),
CollapsedLoopVarDecls);

// Check init.
Stmt *Init = For ? For->getInit() : CXXFor->getBeginStmt();
Expand Down Expand Up @@ -9475,6 +9561,39 @@ static Expr *buildPostUpdate(Sema &S, ArrayRef<Expr *> PostUpdates) {
return PostUpdate;
}

/// Look for variables declared in the body parts of a for-loop nest. Used
/// for verifying loop nest structure before performing a loop collapse
/// operation.
class ForVarDeclFinder final : public RecursiveASTVisitor<ForVarDeclFinder> {
int NestingDepth = 0;
llvm::SmallPtrSetImpl<const Decl *> &VarDecls;

public:
explicit ForVarDeclFinder(llvm::SmallPtrSetImpl<const Decl *> &VD)
: VarDecls(VD) {}

bool VisitForStmt(ForStmt *F) {
++NestingDepth;
TraverseStmt(F->getBody());
--NestingDepth;
return false;
}

bool VisitCXXForRangeStmt(CXXForRangeStmt *RF) {
++NestingDepth;
TraverseStmt(RF->getBody());
--NestingDepth;
return false;
}

bool VisitVarDecl(VarDecl *D) {
Decl *C = D->getCanonicalDecl();
if (NestingDepth > 0)
VarDecls.insert(C);
return true;
}
};

/// Called on a for stmt to check itself and nested loops (if any).
/// \return Returns 0 if one of the collapsed stmts is not canonical for loop,
/// number of collapsed loops otherwise.
Expand All @@ -9487,13 +9606,17 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
unsigned NestedLoopCount = 1;
bool SupportsNonPerfectlyNested = (SemaRef.LangOpts.OpenMP >= 50) &&
!isOpenMPLoopTransformationDirective(DKind);
llvm::SmallPtrSet<const Decl *, 4> CollapsedLoopVarDecls;

if (CollapseLoopCountExpr) {
// Found 'collapse' clause - calculate collapse number.
Expr::EvalResult Result;
if (!CollapseLoopCountExpr->isValueDependent() &&
CollapseLoopCountExpr->EvaluateAsInt(Result, SemaRef.getASTContext())) {
NestedLoopCount = Result.Val.getInt().getLimitedValue();

ForVarDeclFinder FVDF{CollapsedLoopVarDecls};
FVDF.TraverseStmt(AStmt);
} else {
Built.clear(/*Size=*/1);
return 1;
Expand Down Expand Up @@ -9531,11 +9654,13 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
SupportsNonPerfectlyNested, NumLoops,
[DKind, &SemaRef, &DSA, NumLoops, NestedLoopCount,
CollapseLoopCountExpr, OrderedLoopCountExpr, &VarsWithImplicitDSA,
&IterSpaces, &Captures](unsigned Cnt, Stmt *CurStmt) {
&IterSpaces, &Captures,
&CollapsedLoopVarDecls](unsigned Cnt, Stmt *CurStmt) {
if (checkOpenMPIterationSpace(
DKind, CurStmt, SemaRef, DSA, Cnt, NestedLoopCount,
NumLoops, CollapseLoopCountExpr, OrderedLoopCountExpr,
VarsWithImplicitDSA, IterSpaces, Captures))
VarsWithImplicitDSA, IterSpaces, Captures,
CollapsedLoopVarDecls))
return true;
if (Cnt > 0 && Cnt >= NestedLoopCount &&
IterSpaces[Cnt].CounterVar) {
Expand Down
40 changes: 40 additions & 0 deletions clang/test/OpenMP/loop_collapse_1.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// RUN: %clang_cc1 -fopenmp -fopenmp-version=50 -verify %s

void func( double *A, int N, int M, int NB ) {
#pragma omp parallel
{
int nblks = (N-1)/NB;
int lnb = ((N-1)/NB)*NB;

#pragma omp for collapse(2)
for (int jblk = 0 ; jblk < nblks ; jblk++ ) {
int jb = (jblk == nblks - 1 ? lnb : NB);
for (int jk = 0; jk < N; jk+=jb) { // expected-error{{cannot use variable 'jb' in collapsed imperfectly-nested loop increment statement}}
}
}

#pragma omp for collapse(2)
for (int a = 0; a < N; a++) {
for (int b = 0; b < M; b++) {
int cx = a+b < NB ? a : b;
for (int c = 0; c < cx; c++) {
}
}
}

#pragma omp for collapse(3)
for (int a = 0; a < N; a++) {
for (int b = 0; b < M; b++) {
int cx = a+b < NB ? a : b;
for (int c = 0; c < cx; c++) { // expected-error{{cannot use variable 'cx' in collapsed imperfectly-nested loop condition statement}}
}
}
}
}
}

int main(void) {
double arr[256];
func (arr, 16, 16, 16);
return 0;
}
80 changes: 80 additions & 0 deletions clang/test/OpenMP/loop_collapse_2.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// RUN: %clang_cc1 -fopenmp -fopenmp-version=50 -verify %s

// We just want to try out a range for statement... this seems a bit OTT.
template<typename T>
class fakevector {
T *contents;
long size;
public:
fakevector(long sz) : size(sz) {
contents = new T[sz];
}
~fakevector() {
delete[] contents;
}
T& operator[](long x) { return contents[x]; }
typedef T *iterator;
fakevector<T>::iterator begin() {
return &contents[0];
}
fakevector<T>::iterator end() {
return &contents[size];
}
};

void func( double *A, int N, int M, int NB ) {
#pragma omp parallel
{
int nblks = (N-1)/NB;
int lnb = ((N-1)/NB)*NB;
#pragma omp for collapse(2)
for (int jblk = 0 ; jblk < nblks ; jblk++ ) {
int jb = (jblk == nblks - 1 ? lnb : NB);
for (int jk = 0; jk < N; jk+=jb) { // expected-error{{cannot use variable 'jb' in collapsed imperfectly-nested loop increment statement}}
}
}

#pragma omp for collapse(2)
for (int a = 0; a < N; a++) {
for (int b = 0; b < M; b++) {
int cx = a+b < NB ? a : b;
for (int c = 0; c < cx; c++) {
}
}
}

fakevector<float> myvec{N};
#pragma omp for collapse(2)
for (auto &a : myvec) {
fakevector<float> myvec3{M};
for (auto &b : myvec3) { // expected-error{{cannot use variable 'myvec3' in collapsed imperfectly-nested loop init statement}}
}
}

fakevector<float> myvec2{M};

#pragma omp for collapse(3)
for (auto &a : myvec) {
for (auto &b : myvec2) {
int cx = a < b ? N : M;
for (int c = 0; c < cx; c++) { // expected-error {{cannot use variable 'cx' in collapsed imperfectly-nested loop condition statement}}
}
}
}

#pragma omp for collapse(3)
for (auto &a : myvec) {
int cx = a < 5 ? M : N;
for (auto &b : myvec2) {
for (int c = 0; c < cx; c++) { // expected-error{{cannot use variable 'cx' in collapsed imperfectly-nested loop condition statement}}
}
}
}
}
}

int main(void) {
double arr[256];
func (arr, 16, 16, 16);
return 0;
}
Loading