[PATCH] D24615: [OpenMP] clang doesnt diagnose if there is a lexical block around a for stmt for OpenMP loops. It is technically not allowed in the OpenMP standard
davidsh created this revision. davidsh added reviewers: carlo.bertolli, arpith-jacob, kkwli0, sfantao, ABataev. davidsh added a subscriber: cfe-commits. #pragma omp for { for(...) } This is technically not allowed by the standard, gcc doesnt allow such code. https://reviews.llvm.org/D24615 Files: lib/Sema/SemaOpenMP.cpp test/OpenMP/for_loop_messages.cpp Index: test/OpenMP/for_loop_messages.cpp === --- test/OpenMP/for_loop_messages.cpp +++ test/OpenMP/for_loop_messages.cpp @@ -353,6 +353,14 @@ } #pragma omp parallel +// expected-error@+2 {{statement after '#pragma omp for' must be a for loop}} +#pragma omp for + { + for (int i = 0; i < 16; ++i) +; + } + +#pragma omp parallel // expected-note@+3 {{loop step is expected to be positive due to this condition}} // expected-error@+2 {{increment expression must cause 'i' to increase on each iteration of OpenMP for loop}} #pragma omp for Index: lib/Sema/SemaOpenMP.cpp === --- lib/Sema/SemaOpenMP.cpp +++ lib/Sema/SemaOpenMP.cpp @@ -5133,7 +5133,9 @@ llvm::MapVector Captures; SmallVector IterSpaces; IterSpaces.resize(NestedLoopCount); - Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true); + Stmt *CurStmt = AStmt; + if (auto CapS = dyn_cast_or_null(CurStmt)) +CurStmt = CapS->getCapturedStmt(); for (unsigned Cnt = 0; Cnt < NestedLoopCount; ++Cnt) { if (CheckOpenMPIterationSpace(DKind, CurStmt, SemaRef, DSA, Cnt, NestedLoopCount, CollapseLoopCountExpr, Index: test/OpenMP/for_loop_messages.cpp === --- test/OpenMP/for_loop_messages.cpp +++ test/OpenMP/for_loop_messages.cpp @@ -353,6 +353,14 @@ } #pragma omp parallel +// expected-error@+2 {{statement after '#pragma omp for' must be a for loop}} +#pragma omp for + { + for (int i = 0; i < 16; ++i) +; + } + +#pragma omp parallel // expected-note@+3 {{loop step is expected to be positive due to this condition}} // expected-error@+2 {{increment expression must cause 'i' to increase on each iteration of OpenMP for loop}} #pragma omp for Index: lib/Sema/SemaOpenMP.cpp === --- lib/Sema/SemaOpenMP.cpp +++ lib/Sema/SemaOpenMP.cpp @@ -5133,7 +5133,9 @@ llvm::MapVector Captures; SmallVector IterSpaces; IterSpaces.resize(NestedLoopCount); - Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true); + Stmt *CurStmt = AStmt; + if (auto CapS = dyn_cast_or_null(CurStmt)) +CurStmt = CapS->getCapturedStmt(); for (unsigned Cnt = 0; Cnt < NestedLoopCount; ++Cnt) { if (CheckOpenMPIterationSpace(DKind, CurStmt, SemaRef, DSA, Cnt, NestedLoopCount, CollapseLoopCountExpr, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24615: [OpenMP] clang doesnt diagnose if there is a lexical block around a for stmt for OpenMP loops. It is technically not allowed in the OpenMP standard
davidsh updated this revision to Diff 73520. davidsh added a comment. Adding a parameter to IgnoreContainers instead of copying the logic. https://reviews.llvm.org/D24615 Files: include/clang/AST/Stmt.h lib/AST/Stmt.cpp lib/Sema/SemaOpenMP.cpp test/OpenMP/for_loop_messages.cpp Index: test/OpenMP/for_loop_messages.cpp === --- test/OpenMP/for_loop_messages.cpp +++ test/OpenMP/for_loop_messages.cpp @@ -353,6 +353,14 @@ } #pragma omp parallel +// expected-error@+2 {{statement after '#pragma omp for' must be a for loop}} +#pragma omp for + { + for (int i = 0; i < 16; ++i) +; + } + +#pragma omp parallel // expected-note@+3 {{loop step is expected to be positive due to this condition}} // expected-error@+2 {{increment expression must cause 'i' to increase on each iteration of OpenMP for loop}} #pragma omp for Index: lib/Sema/SemaOpenMP.cpp === --- lib/Sema/SemaOpenMP.cpp +++ lib/Sema/SemaOpenMP.cpp @@ -5133,7 +5133,8 @@ llvm::MapVector Captures; SmallVector IterSpaces; IterSpaces.resize(NestedLoopCount); - Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true); + Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true, + /* IgnoreCompound */ false); for (unsigned Cnt = 0; Cnt < NestedLoopCount; ++Cnt) { if (CheckOpenMPIterationSpace(DKind, CurStmt, SemaRef, DSA, Cnt, NestedLoopCount, CollapseLoopCountExpr, Index: lib/AST/Stmt.cpp === --- lib/AST/Stmt.cpp +++ lib/AST/Stmt.cpp @@ -111,20 +111,24 @@ return s; } -/// \brief Skip no-op (attributed, compound) container stmts and skip captured -/// stmt at the top, if \a IgnoreCaptured is true. -Stmt *Stmt::IgnoreContainers(bool IgnoreCaptured) { +/// \brief Skip no-op (attributed, compound if \a IgnoreCompound is true) +/// container stmts and skip captured stmt at the top, if \a IgnoreCaptured +/// is true. +Stmt *Stmt::IgnoreContainers(bool IgnoreCaptured, bool IgnoreCompound) { Stmt *S = this; if (IgnoreCaptured) if (auto CapS = dyn_cast_or_null(S)) S = CapS->getCapturedStmt(); while (true) { if (auto AS = dyn_cast_or_null(S)) S = AS->getSubStmt(); -else if (auto CS = dyn_cast_or_null(S)) { - if (CS->size() != 1) -break; - S = CS->body_back(); +else if (IgnoreCompound) { + if (auto CS = dyn_cast_or_null(S)) { +if (CS->size() != 1) + break; +S = CS->body_back(); + } else + break; } else break; } Index: include/clang/AST/Stmt.h === --- include/clang/AST/Stmt.h +++ include/clang/AST/Stmt.h @@ -388,9 +388,11 @@ /// statement, such as ExprWithCleanups or ImplicitCastExpr nodes. Stmt *IgnoreImplicit(); - /// \brief Skip no-op (attributed, compound) container stmts and skip captured - /// stmt at the top, if \a IgnoreCaptured is true. - Stmt *IgnoreContainers(bool IgnoreCaptured = false); + /// \brief Skip no-op (attributed, compound if \a IgnoreCompound is true) + /// container stmts and skip captured stmt at the top, if \a IgnoreCaptured + /// is true. + Stmt *IgnoreContainers(bool IgnoreCaptured = false, + bool IgnoreCompound = true); const Stmt *stripLabelLikeStatements() const; Stmt *stripLabelLikeStatements() { Index: test/OpenMP/for_loop_messages.cpp === --- test/OpenMP/for_loop_messages.cpp +++ test/OpenMP/for_loop_messages.cpp @@ -353,6 +353,14 @@ } #pragma omp parallel +// expected-error@+2 {{statement after '#pragma omp for' must be a for loop}} +#pragma omp for + { + for (int i = 0; i < 16; ++i) +; + } + +#pragma omp parallel // expected-note@+3 {{loop step is expected to be positive due to this condition}} // expected-error@+2 {{increment expression must cause 'i' to increase on each iteration of OpenMP for loop}} #pragma omp for Index: lib/Sema/SemaOpenMP.cpp === --- lib/Sema/SemaOpenMP.cpp +++ lib/Sema/SemaOpenMP.cpp @@ -5133,7 +5133,8 @@ llvm::MapVector Captures; SmallVector IterSpaces; IterSpaces.resize(NestedLoopCount); - Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true); + Stmt *CurStmt = AStmt->IgnoreContainers(/* IgnoreCaptured */ true, + /* IgnoreCompound */ false); for (unsigned Cnt = 0; Cnt < NestedLoopCount; ++Cnt) { if (CheckOpenMPIterationSpace(DKind, CurStmt, SemaRef, DSA, Cnt, NestedLoopCount, CollapseLoopCountExpr, Index: lib/AST/Stmt.cpp === --- lib/AST/Stmt.
[PATCH] D24615: [OpenMP] clang doesnt diagnose if there is a lexical block around a for stmt for OpenMP loops. It is technically not allowed in the OpenMP standard
kkwli0 added a comment. Should we issue a warning message in this case? https://reviews.llvm.org/D24615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24615: [OpenMP] clang doesnt diagnose if there is a lexical block around a for stmt for OpenMP loops. It is technically not allowed in the OpenMP standard
ABataev added a comment. In https://reviews.llvm.org/D24615#561532, @kkwli0 wrote: > Should we issue a warning message in this case? Agree https://reviews.llvm.org/D24615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24615: [OpenMP] clang doesnt diagnose if there is a lexical block around a for stmt for OpenMP loops. It is technically not allowed in the OpenMP standard
ABataev added inline comments. Comment at: lib/Sema/SemaOpenMP.cpp:5137-5138 @@ -5137,1 +5136,4 @@ + Stmt *CurStmt = AStmt; + if (auto CapS = dyn_cast_or_null(CurStmt)) +CurStmt = CapS->getCapturedStmt(); for (unsigned Cnt = 0; Cnt < NestedLoopCount; ++Cnt) { You also should skip all AttributedStmts. https://reviews.llvm.org/D24615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits