<html><body>
<p><font size="2" face="sans-serif">Hi Alexey,</font><br>
<br>
<tt><font size="2">Alexey Bataev <a.bataev@hotmail.com> wrote on 06/26/2014 11:29:04 PM:<br>
<br>
> From: Alexey Bataev <a.bataev@hotmail.com></font></tt><br>
<tt><font size="2">> To: cfe-commits@cs.uiuc.edu, Samuel F Antao/Watson/IBM@IBMUS</font></tt><br>
<tt><font size="2">> Date: 06/26/2014 11:29 PM</font></tt><br>
<tt><font size="2">> Subject: Re: r211566 - [OPENMP] Added initial checking of nesting of<br>
> OpenMP regions.</font></tt><br>
<tt><font size="2">> <br>
> Hi Samuel,<br>
> Thanks for the review.<br>
> > a) In in SemaOpenMP.cpp lines 936, 2292, 2404, 2797<br>
> > isOpenMPWorksharingDirective() dominates all the other elements of the<br>
> > conditional i.e. the Boolean value returned by this function will be the<br>
> > same as the remaining components of the conditional. So, the other<br>
> > components can be removed.<br>
> ><br>
> This is done for future combined constructs like 'omp parallel for' <br>
> which is parallel and worksharing at same time.</font></tt><br>
<br>
<tt><font size="2">Ok. Makes sense.</font></tt><br>
<tt><font size="2"><br>
> > b) As other nesting constraints are added in future commits, I expect the<br>
> > complexity/readability of CheckNestingOfRegions() to increase. For now it<br>
> > is okay, but I think that maintaining some static information about all the<br>
> > valid/invalid nestings will simplify the way this function is implemented<br>
> > and improve readability.<br>
> What kind of static info do you mean?</font></tt><br>
<br>
<tt><font size="2">I meant some sort of static table whose elements contain the valid nestings, like:</font></tt><br>
<br>
<tt><font size="2">[Parent Directive Kind, Current Directive Kind, Closely|No-Closely|Both ]</font></tt><br>
<br>
<tt><font size="2">If the nesting under test is not in the table it would fail. As I have mentioned, the code looks okay</font></tt><br>
<tt><font size="2">right now, but in the future if we see that function starts to get messy due to the nestings combinations</font></tt><br>
<tt><font size="2">one can implement a solution like this. </font></tt><br>
<tt><font size="2"><br>
> > c) For the sake of completeness I think nestings_of_regions.cpp should also<br>
> > have a case where a nesting exists but it is not closely nested. It may not<br>
> > make a lot of sense from an application point of view, but I think it<br>
> > should be useful to have something like this in the testcase:<br>
> ><br>
> > #pragma omp parallel<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>
> > #pragma omp for<br>
> > for (int i = 0; i < 10; ++i);<br>
> > }<br>
> > }<br>
> ><br>
> Ok, I'll add such tests.</font></tt><br>
<br>
<tt><font size="2">Ok, thanks.</font></tt><br>
<tt><font size="2"><br>
> > Question: Why the template version of foo in the testcase?<br>
> Templates are very tricky constructs and actually we have to test all <br>
> directives inside template constructs to be sure that we handle <br>
> templates properly and instantiation and specialization works for each <br>
> directive.</font></tt><br>
<br>
<tt><font size="2">Got it.</font></tt><br>
<tt><font size="2"><br>
> <br>
> Best regards,<br>
> Alexey Bataev<br>
> =============<br>
> Software Engineer<br>
> Intel Compiler Team<br>
> <br>
> 27.06.2014 2:48, cfe-commits-request@cs.uiuc.edu пишет:<br>
> > Send cfe-commits mailing list submissions to<br>
> > cfe-commits@cs.uiuc.edu<br>
> ><br>
> > To subscribe or unsubscribe via the World Wide Web, visit<br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> > or, via email, send a message with subject or body 'help' to<br>
> > cfe-commits-request@cs.uiuc.edu<br>
> ><br>
> > You can reach the person managing the list at<br>
> > cfe-commits-owner@cs.uiuc.edu<br>
> ><br>
> > When replying, please edit your Subject line so it is more specific<br>
> > than "Re: Contents of cfe-commits digest..."<br>
> ><br>
> ><br>
> > Today's Topics:<br>
> ><br>
> > 1. Re: r211566 - [OPENMP] Added initial checking of nesting of<br>
> > OpenMP regions. (Samuel F Antao)<br>
> ><br>
> ><br>
> > ----------------------------------------------------------------------<br>
> ><br>
> > Message: 1<br>
> > Date: Thu, 26 Jun 2014 18:48:56 -0400<br>
> > From: Samuel F Antao <sfantao@us.ibm.com><br>
> > To: cfe-commits@cs.uiuc.edu<br>
> > Cc: cfe-commits-bounces@cs.uiuc.edu<br>
> > Subject: Re: r211566 - [OPENMP] Added initial checking of nesting of<br>
> > OpenMP regions.<br>
> > Message-ID:<br>
> > <OFFB19983E.4AA6C883-ON85257D03.007A5672-85257D03.007D5442@us.ibm.com><br>
> > Content-Type: text/plain; charset="us-ascii"<br>
> ><br>
> ><br>
> > Hi Alexey,<br>
> ><br>
> > I took a close look into this commit and I have some suggestions to make:<br>
> ><br>
> > a) In in SemaOpenMP.cpp lines 936, 2292, 2404, 2797<br>
> > isOpenMPWorksharingDirective() dominates all the other elements of the<br>
> > conditional i.e. the Boolean value returned by this function will be the<br>
> > same as the remaining components of the conditional. So, the other<br>
> > components can be removed.<br>
> ><br>
> > b) As other nesting constraints are added in future commits, I expect the<br>
> > complexity/readability of CheckNestingOfRegions() to increase. For now it<br>
> > is okay, but I think that maintaining some static information about all the<br>
> > valid/invalid nestings will simplify the way this function is implemented<br>
> > and improve readability.<br>
> ><br>
> > c) For the sake of completeness I think nestings_of_regions.cpp should also<br>
> > have a case where a nesting exists but it is not closely nested. It may not<br>
> > make a lot of sense from an application point of view, but I think it<br>
> > should be useful to have something like this in the testcase:<br>
> ><br>
> > #pragma omp parallel<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>
> > #pragma omp for<br>
> > for (int i = 0; i < 10; ++i);<br>
> > }<br>
> > }<br>
> ><br>
> > Question: Why the template version of foo in the testcase?<br>
> ><br>
> > Regards,<br>
> > --Samuel Antao<br>
> ><br>
> ><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>
> > <br>
> ==============================================================================<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>
> > <br>
> ==============================================================================<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<br>
> > 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) <<<br>
> > true<br>
> >> + << getOpenMPDirectiveName(CurrentRegion) <<<br>
> >> ShouldBeInParallelRegion;<br>
> >> + return true;<br>
> >> + }<br>
> >> + }<br>
> >> + return false;<br>
> >> +}<br>
> >> +<br>
> >> StmtResult Sema::ActOnOpenMPExecutableDirective(OpenMPDirectiveKind<br>
> > 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<br>
> > expected");<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<br>
> > the<br>
> >> // worksharing regions arising from the worksharing<br>
> >> construct ever bind<br>
> >> // to any of the parallel regions arising from the parallel<br>
> > 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<br>
> > 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<br>
> > 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>
> > <br>
> ==============================================================================<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>
> > <br>
> ------------------------------------------------------------------------------<br>
> ><br>
> >> svn:eol-style = native<br>
> >><br>
> >> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> >><br>
> > <br>
> ------------------------------------------------------------------------------<br>
> ><br>
> >> svn:keywords = Author Date Id Rev URL<br>
> >><br>
> >> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp<br>
> >><br>
> > <br>
> ------------------------------------------------------------------------------<br>
> ><br>
> >> svn:mime-type = text/plain<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> ------------------------------<br>
> >><br>
> > -------------- next part --------------<br>
> > An HTML attachment was scrubbed...<br>
> > URL: <<a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/">http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/</a><br>
> 20140626/0739f9d2/attachment.html><br>
> ><br>
> > ------------------------------<br>
> ><br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > cfe-commits@cs.uiuc.edu<br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> ><br>
> ><br>
> > End of cfe-commits Digest, Vol 84, Issue 504<br>
> > ********************************************<br>
> <br>
</font></tt></body></html>