<html><body>
<p><font size="2" face="sans-serif">Hi Alexey,</font><br>
<br>
<font size="2" face="sans-serif">I took a close look into this commit and I have some suggestions to make:</font><br>
<br>
<font size="2" face="sans-serif">a) In in SemaOpenMP.cpp lines 936, 2292, 2404, 2797 </font><font size="2" face="sans-serif">isOpenMPWorksharingDirective() dominates all the other elements of the conditional i.e. the Boolean value returned by this function will be the same as the remaining components of the conditional. So, the other components can be removed. </font><br>
<br>
<font size="2" face="sans-serif">b) As other nesting constraints are added in future commits, I expect the complexity/readability of CheckNestingOfRegions() to increase. For now it is okay, but I think that maintaining some static information about all the valid/invalid nestings will simplify the way this function is implemented and improve readability.</font><br>
<br>
<font size="2" face="sans-serif">c) For the sake of completeness I think nestings_of_regions.cpp should also have a case where a nesting exists but it is not closely nested. It may not make a lot of sense from an application point of view, but I think it should be useful to have something like this in the testcase: </font><br>
<br>
<tt><font size="2">#pragma omp parallel</font></tt><br>
<tt><font size="2">#pragma omp for</font></tt><br>
<tt><font size="2"> for (int i = 0; i < 10; ++i){</font></tt><br>
<tt><font size="2">#pragma omp parallel</font></tt><br>
<tt><font size="2"> for (int i = 0; i < 10; ++i){</font></tt><br>
<tt><font size="2">#pragma omp for</font></tt><br>
<tt><font size="2"> for (int i = 0; i < 10; ++i);</font></tt><br>
<tt><font size="2"> }</font></tt><br>
<tt><font size="2"> }</font></tt><br>
<br>
<tt><font size="2">Question: Why the template version of foo in the testcase?</font></tt><br>
<br>
<tt><font size="2">Regards,</font></tt><br>
<tt><font size="2">--Samuel Antao</font></tt><br>
<br>
<tt><font size="2"><br>
> Date: Tue, 24 Jun 2014 04:39:47 -0000<br>
> From: Alexey Bataev <a.bataev@hotmail.com><br>
> To: cfe-commits@cs.uiuc.edu<br>
> Subject: r211566 - [OPENMP] Added initial checking of nesting of<br>
> OpenMP regions.<br>
> Message-ID: <20140624043948.3A2702A6C029@llvm.org><br>
> Content-Type: text/plain; charset="utf-8"<br>
> <br>
> Author: abataev<br>
> Date: Mon Jun 23 23:39:47 2014<br>
> New Revision: 211566<br>
> <br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=211566&view=rev">http://llvm.org/viewvc/llvm-project?rev=211566&view=rev</a><br>
> Log:<br>
> [OPENMP] Added initial checking of nesting of OpenMP regions.<br>
> <br>
> Added:<br>
> cfe/trunk/test/OpenMP/nesting_of_regions.cpp (with props)<br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> cfe/trunk/lib/Sema/SemaOpenMP.cpp<br>
> <br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/</a><br>
> Basic/DiagnosticSemaKinds.td?rev=211566&r1=211565&r2=211566&view=diff<br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 23 <br>
> 23:39:47 2014<br>
> @@ -7096,6 +7096,11 @@ def err_omp_reduction_in_task : Error<<br>
> "reduction variables may not be accessed in an explicit task">;<br>
> def err_omp_reduction_id_not_compatible : Error<<br>
> "variable of type %0 is not valid for specified reduction operation">;<br>
> +def err_omp_prohibited_region : Error<<br>
> + "region cannot be%select{| closely}0 nested inside '%1' region"<br>
> + "%select{|; perhaps you forget to enclose 'omp %3' directive into<br>
> a parallel region?}2">;<br>
> +def err_omp_prohibited_region_simd : Error<<br>
> + "OpenMP constructs may not be nested inside a simd region">;<br>
> } // end of OpenMP category<br>
> <br>
> let CategoryName = "Related Result Type Issue" in {<br>
> <br>
> Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/</a><br>
> SemaOpenMP.cpp?rev=211566&r1=211565&r2=211566&view=diff<br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Mon Jun 23 23:39:47 2014<br>
> @@ -152,6 +152,12 @@ public:<br>
> OpenMPDirectiveKind getCurrentDirective() const {<br>
> return Stack.back().Directive;<br>
> }<br>
> + /// \brief Returns parent directive.<br>
> + OpenMPDirectiveKind getParentDirective() const {<br>
> + if (Stack.size() > 2)<br>
> + return Stack[Stack.size() - 2].Directive;<br>
> + return OMPD_unknown;<br>
> + }<br>
> <br>
> /// \brief Set default data sharing attribute to none.<br>
> void setDefaultDSANone() { Stack.back().DefaultAttr = DSA_none; }<br>
> @@ -912,6 +918,41 @@ void Sema::ActOnOpenMPRegionStart(OpenMP<br>
> }<br>
> }<br>
> <br>
> +bool CheckNestingOfRegions(Sema &SemaRef, DSAStackTy *Stack,<br>
> + OpenMPDirectiveKind CurrentRegion,<br>
> + SourceLocation StartLoc) {<br>
> + if (Stack->getCurScope()) {<br>
> + auto ParentRegion = Stack->getParentDirective();<br>
> + bool NestingProhibited = false;<br>
> + bool CloseNesting = true;<br>
> + bool ShouldBeInParallelRegion = false;<br>
> + if (isOpenMPSimdDirective(ParentRegion)) {<br>
> + // OpenMP [2.16, Nesting of Regions]<br>
> + // OpenMP constructs may not be nested inside a simd region.<br>
> + SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region_simd);<br>
> + return true;<br>
> + }<br>
> + if (isOpenMPWorksharingDirective(CurrentRegion) &&<br>
> + !isOpenMPParallelDirective(CurrentRegion) &&<br>
> + !isOpenMPSimdDirective(CurrentRegion)) {<br>
> + // OpenMP [2.16, Nesting of Regions]<br>
> + // A worksharing region may not be closely nested inside a worksharing,<br>
> + // explicit task, critical, ordered, atomic, or master region.<br>
> + // TODO<br>
> + NestingProhibited = isOpenMPWorksharingDirective(ParentRegion) &&<br>
> + !isOpenMPSimdDirective(ParentRegion);<br>
> + ShouldBeInParallelRegion = true;<br>
> + }<br>
> + if (NestingProhibited) {<br>
> + SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region)<br>
> + << CloseNesting << getOpenMPDirectiveName(ParentRegion) << true<br>
> + << getOpenMPDirectiveName(CurrentRegion) << <br>
> ShouldBeInParallelRegion;<br>
> + return true;<br>
> + }<br>
> + }<br>
> + return false;<br>
> +}<br>
> +<br>
> StmtResult Sema::ActOnOpenMPExecutableDirective(OpenMPDirectiveKind Kind,<br>
> ArrayRef<OMPClause <br>
> *> Clauses,<br>
> Stmt *AStmt,<br>
> @@ -920,6 +961,8 @@ StmtResult Sema::ActOnOpenMPExecutableDi<br>
> assert(AStmt && isa<CapturedStmt>(AStmt) && "Captured statement expected");<br>
> <br>
> StmtResult Res = StmtError();<br>
> + if (CheckNestingOfRegions(*this, DSAStack, Kind, StartLoc))<br>
> + return StmtError();<br>
> <br>
> // Check default data sharing attributes for referenced variables.<br>
> DSAAttrChecker DSAChecker(DSAStack, *this, cast<CapturedStmt>(AStmt));<br>
> @@ -2246,7 +2289,8 @@ OMPClause *Sema::ActOnOpenMPFirstprivate<br>
> // in a firstprivate clause on a worksharing construct if any of the<br>
> // worksharing regions arising from the worksharing <br>
> construct ever bind<br>
> // to any of the parallel regions arising from the parallel construct.<br>
> - if (isOpenMPWorksharingDirective(CurrDir)) {<br>
> + if (isOpenMPWorksharingDirective(CurrDir) &&<br>
> + !isOpenMPParallelDirective(CurrDir)) {<br>
> DVar = DSAStack->getImplicitDSA(VD);<br>
> if (DVar.CKind != OMPC_shared) {<br>
> Diag(ELoc, diag::err_omp_required_access)<br>
> @@ -2357,7 +2401,8 @@ OMPClause *Sema::ActOnOpenMPLastprivateC<br>
> // lastprivate clause on a worksharing construct if any of the <br>
> corresponding<br>
> // worksharing regions ever binds to any of the corresponding parallel<br>
> // regions.<br>
> - if (isOpenMPWorksharingDirective(CurrDir)) {<br>
> + if (isOpenMPWorksharingDirective(CurrDir) &&<br>
> + !isOpenMPParallelDirective(CurrDir)) {<br>
> DVar = DSAStack->getImplicitDSA(VD);<br>
> if (DVar.CKind != OMPC_shared) {<br>
> Diag(ELoc, diag::err_omp_required_access)<br>
> @@ -2748,7 +2793,8 @@ OMPClause *Sema::ActOnOpenMPReductionCla<br>
> // construct must be shared in the parallel regions to which any of the<br>
> // worksharing regions arising from the worksharing construct bind.<br>
> OpenMPDirectiveKind CurrDir = DSAStack->getCurrentDirective();<br>
> - if (isOpenMPWorksharingDirective(CurrDir)) {<br>
> + if (isOpenMPWorksharingDirective(CurrDir) &&<br>
> + !isOpenMPParallelDirective(CurrDir)) {<br>
> DVar = DSAStack->getImplicitDSA(VD);<br>
> if (DVar.CKind != OMPC_shared) {<br>
> Diag(ELoc, diag::err_omp_required_access)<br>
> <br>
> Added: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/</a><br>
> nesting_of_regions.cpp?rev=211566&view=auto<br>
> ==============================================================================<br>
> --- cfe/trunk/test/OpenMP/nesting_of_regions.cpp (added)<br>
> +++ cfe/trunk/test/OpenMP/nesting_of_regions.cpp Mon Jun 23 23:39:47 2014<br>
> @@ -0,0 +1,82 @@<br>
> +// RUN: %clang_cc1 -fsyntax-only -fopenmp=libiomp5 -verify %s<br>
> +<br>
> +template <class T><br>
> +void foo() {<br>
> +#pragma omp parallel<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i);<br>
> +#pragma omp parallel<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i);<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp for // expected-error {{OpenMP constructs may not be <br>
> nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp simd // expected-error {{OpenMP constructs may not be <br>
> nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp parallel // expected-error {{OpenMP constructs may not <br>
> be nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp for // expected-error {{region cannot be closely nested<br>
> inside 'for' region; perhaps you forget to enclose 'omp for' <br>
> directive into a parallel region?}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp parallel<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +}<br>
> +<br>
> +void foo() {<br>
> +#pragma omp parallel<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i);<br>
> +#pragma omp parallel<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i);<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp for // expected-error {{OpenMP constructs may not be <br>
> nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp simd // expected-error {{OpenMP constructs may not be <br>
> nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp parallel // expected-error {{OpenMP constructs may not <br>
> be nested inside a simd region}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp for // expected-error {{region cannot be closely nested<br>
> inside 'for' region; perhaps you forget to enclose 'omp for' <br>
> directive into a parallel region?}}<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp simd<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> +#pragma omp for<br>
> + for (int i = 0; i < 10; ++i) {<br>
> +#pragma omp parallel<br>
> + for (int i = 0; i < 10; ++i);<br>
> + }<br>
> + return foo<int>();<br>
> +}<br>
> +<br>
> <br>
> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> ------------------------------------------------------------------------------<br>
> svn:eol-style = native<br>
> <br>
> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> ------------------------------------------------------------------------------<br>
> svn:keywords = Author Date Id Rev URL<br>
> <br>
> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> ------------------------------------------------------------------------------<br>
> svn:mime-type = text/plain<br>
> <br>
> <br>
> <br>
> <br>
> ------------------------------<br>
> <br>
<br>
</font></tt></body></html>