r211566 - [OPENMP] Added initial checking of nesting of OpenMP regions.
Alexey Bataev
a.bataev at hotmail.com
Thu Jun 26 20:29:04 PDT 2014
Hi Samuel,
Thanks for the review.
> a) In in SemaOpenMP.cpp lines 936, 2292, 2404, 2797
> 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.
>
This is done for future combined constructs like 'omp parallel for'
which is parallel and worksharing at same time.
> 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.
What kind of static info do you mean?
> 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:
>
> #pragma omp parallel
> #pragma omp for
> for (int i = 0; i < 10; ++i){
> #pragma omp parallel
> for (int i = 0; i < 10; ++i){
> #pragma omp for
> for (int i = 0; i < 10; ++i);
> }
> }
>
Ok, I'll add such tests.
> Question: Why the template version of foo in the testcase?
Templates are very tricky constructs and actually we have to test all
directives inside template constructs to be sure that we handle
templates properly and instantiation and specialization works for each
directive.
Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team
27.06.2014 2:48, cfe-commits-request at cs.uiuc.edu пишет:
> Send cfe-commits mailing list submissions to
> cfe-commits at cs.uiuc.edu
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> or, via email, send a message with subject or body 'help' to
> cfe-commits-request at cs.uiuc.edu
>
> You can reach the person managing the list at
> cfe-commits-owner at cs.uiuc.edu
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of cfe-commits digest..."
>
>
> Today's Topics:
>
> 1. Re: r211566 - [OPENMP] Added initial checking of nesting of
> OpenMP regions. (Samuel F Antao)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Thu, 26 Jun 2014 18:48:56 -0400
> From: Samuel F Antao <sfantao at us.ibm.com>
> To: cfe-commits at cs.uiuc.edu
> Cc: cfe-commits-bounces at cs.uiuc.edu
> Subject: Re: r211566 - [OPENMP] Added initial checking of nesting of
> OpenMP regions.
> Message-ID:
> <OFFB19983E.4AA6C883-ON85257D03.007A5672-85257D03.007D5442 at us.ibm.com>
> Content-Type: text/plain; charset="us-ascii"
>
>
> Hi Alexey,
>
> I took a close look into this commit and I have some suggestions to make:
>
> a) In in SemaOpenMP.cpp lines 936, 2292, 2404, 2797
> 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.
>
> 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.
>
> 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:
>
> #pragma omp parallel
> #pragma omp for
> for (int i = 0; i < 10; ++i){
> #pragma omp parallel
> for (int i = 0; i < 10; ++i){
> #pragma omp for
> for (int i = 0; i < 10; ++i);
> }
> }
>
> Question: Why the template version of foo in the testcase?
>
> Regards,
> --Samuel Antao
>
>
>> Date: Tue, 24 Jun 2014 04:39:47 -0000
>> From: Alexey Bataev <a.bataev at hotmail.com>
>> To: cfe-commits at cs.uiuc.edu
>> Subject: r211566 - [OPENMP] Added initial checking of nesting of
>> OpenMP regions.
>> Message-ID: <20140624043948.3A2702A6C029 at llvm.org>
>> Content-Type: text/plain; charset="utf-8"
>>
>> Author: abataev
>> Date: Mon Jun 23 23:39:47 2014
>> New Revision: 211566
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=211566&view=rev
>> Log:
>> [OPENMP] Added initial checking of nesting of OpenMP regions.
>>
>> Added:
>> cfe/trunk/test/OpenMP/nesting_of_regions.cpp (with props)
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaOpenMP.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>> Basic/DiagnosticSemaKinds.td?rev=211566&r1=211565&r2=211566&view=diff
>>
> ==============================================================================
>
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jun 23
>> 23:39:47 2014
>> @@ -7096,6 +7096,11 @@ def err_omp_reduction_in_task : Error<
>> "reduction variables may not be accessed in an explicit task">;
>> def err_omp_reduction_id_not_compatible : Error<
>> "variable of type %0 is not valid for specified reduction operation">;
>> +def err_omp_prohibited_region : Error<
>> + "region cannot be%select{| closely}0 nested inside '%1' region"
>> + "%select{|; perhaps you forget to enclose 'omp %3' directive into
>> a parallel region?}2">;
>> +def err_omp_prohibited_region_simd : Error<
>> + "OpenMP constructs may not be nested inside a simd region">;
>> } // end of OpenMP category
>>
>> let CategoryName = "Related Result Type Issue" in {
>>
>> Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
>> SemaOpenMP.cpp?rev=211566&r1=211565&r2=211566&view=diff
>>
> ==============================================================================
>
>> --- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Mon Jun 23 23:39:47 2014
>> @@ -152,6 +152,12 @@ public:
>> OpenMPDirectiveKind getCurrentDirective() const {
>> return Stack.back().Directive;
>> }
>> + /// \brief Returns parent directive.
>> + OpenMPDirectiveKind getParentDirective() const {
>> + if (Stack.size() > 2)
>> + return Stack[Stack.size() - 2].Directive;
>> + return OMPD_unknown;
>> + }
>>
>> /// \brief Set default data sharing attribute to none.
>> void setDefaultDSANone() { Stack.back().DefaultAttr = DSA_none; }
>> @@ -912,6 +918,41 @@ void Sema::ActOnOpenMPRegionStart(OpenMP
>> }
>> }
>>
>> +bool CheckNestingOfRegions(Sema &SemaRef, DSAStackTy *Stack,
>> + OpenMPDirectiveKind CurrentRegion,
>> + SourceLocation StartLoc) {
>> + if (Stack->getCurScope()) {
>> + auto ParentRegion = Stack->getParentDirective();
>> + bool NestingProhibited = false;
>> + bool CloseNesting = true;
>> + bool ShouldBeInParallelRegion = false;
>> + if (isOpenMPSimdDirective(ParentRegion)) {
>> + // OpenMP [2.16, Nesting of Regions]
>> + // OpenMP constructs may not be nested inside a simd region.
>> + SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region_simd);
>> + return true;
>> + }
>> + if (isOpenMPWorksharingDirective(CurrentRegion) &&
>> + !isOpenMPParallelDirective(CurrentRegion) &&
>> + !isOpenMPSimdDirective(CurrentRegion)) {
>> + // OpenMP [2.16, Nesting of Regions]
>> + // A worksharing region may not be closely nested inside a
> worksharing,
>> + // explicit task, critical, ordered, atomic, or master region.
>> + // TODO
>> + NestingProhibited = isOpenMPWorksharingDirective(ParentRegion) &&
>> + !isOpenMPSimdDirective(ParentRegion);
>> + ShouldBeInParallelRegion = true;
>> + }
>> + if (NestingProhibited) {
>> + SemaRef.Diag(StartLoc, diag::err_omp_prohibited_region)
>> + << CloseNesting << getOpenMPDirectiveName(ParentRegion) <<
> true
>> + << getOpenMPDirectiveName(CurrentRegion) <<
>> ShouldBeInParallelRegion;
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>> +
>> StmtResult Sema::ActOnOpenMPExecutableDirective(OpenMPDirectiveKind
> Kind,
>> ArrayRef<OMPClause
>> *> Clauses,
>> Stmt *AStmt,
>> @@ -920,6 +961,8 @@ StmtResult Sema::ActOnOpenMPExecutableDi
>> assert(AStmt && isa<CapturedStmt>(AStmt) && "Captured statement
> expected");
>> StmtResult Res = StmtError();
>> + if (CheckNestingOfRegions(*this, DSAStack, Kind, StartLoc))
>> + return StmtError();
>>
>> // Check default data sharing attributes for referenced variables.
>> DSAAttrChecker DSAChecker(DSAStack, *this, cast<CapturedStmt>(AStmt));
>> @@ -2246,7 +2289,8 @@ OMPClause *Sema::ActOnOpenMPFirstprivate
>> // in a firstprivate clause on a worksharing construct if any of
> the
>> // worksharing regions arising from the worksharing
>> construct ever bind
>> // to any of the parallel regions arising from the parallel
> construct.
>> - if (isOpenMPWorksharingDirective(CurrDir)) {
>> + if (isOpenMPWorksharingDirective(CurrDir) &&
>> + !isOpenMPParallelDirective(CurrDir)) {
>> DVar = DSAStack->getImplicitDSA(VD);
>> if (DVar.CKind != OMPC_shared) {
>> Diag(ELoc, diag::err_omp_required_access)
>> @@ -2357,7 +2401,8 @@ OMPClause *Sema::ActOnOpenMPLastprivateC
>> // lastprivate clause on a worksharing construct if any of the
>> corresponding
>> // worksharing regions ever binds to any of the corresponding
> parallel
>> // regions.
>> - if (isOpenMPWorksharingDirective(CurrDir)) {
>> + if (isOpenMPWorksharingDirective(CurrDir) &&
>> + !isOpenMPParallelDirective(CurrDir)) {
>> DVar = DSAStack->getImplicitDSA(VD);
>> if (DVar.CKind != OMPC_shared) {
>> Diag(ELoc, diag::err_omp_required_access)
>> @@ -2748,7 +2793,8 @@ OMPClause *Sema::ActOnOpenMPReductionCla
>> // construct must be shared in the parallel regions to which any of
> the
>> // worksharing regions arising from the worksharing construct bind.
>> OpenMPDirectiveKind CurrDir = DSAStack->getCurrentDirective();
>> - if (isOpenMPWorksharingDirective(CurrDir)) {
>> + if (isOpenMPWorksharingDirective(CurrDir) &&
>> + !isOpenMPParallelDirective(CurrDir)) {
>> DVar = DSAStack->getImplicitDSA(VD);
>> if (DVar.CKind != OMPC_shared) {
>> Diag(ELoc, diag::err_omp_required_access)
>>
>> Added: cfe/trunk/test/OpenMP/nesting_of_regions.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/
>> nesting_of_regions.cpp?rev=211566&view=auto
>>
> ==============================================================================
>
>> --- cfe/trunk/test/OpenMP/nesting_of_regions.cpp (added)
>> +++ cfe/trunk/test/OpenMP/nesting_of_regions.cpp Mon Jun 23 23:39:47 2014
>> @@ -0,0 +1,82 @@
>> +// RUN: %clang_cc1 -fsyntax-only -fopenmp=libiomp5 -verify %s
>> +
>> +template <class T>
>> +void foo() {
>> +#pragma omp parallel
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i);
>> +#pragma omp parallel
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i);
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp for // expected-error {{OpenMP constructs may not be
>> nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp simd // expected-error {{OpenMP constructs may not be
>> nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp parallel // expected-error {{OpenMP constructs may not
>> be nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp for // expected-error {{region cannot be closely nested
>> inside 'for' region; perhaps you forget to enclose 'omp for'
>> directive into a parallel region?}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp parallel
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +}
>> +
>> +void foo() {
>> +#pragma omp parallel
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i);
>> +#pragma omp parallel
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i);
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp for // expected-error {{OpenMP constructs may not be
>> nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp simd // expected-error {{OpenMP constructs may not be
>> nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp parallel // expected-error {{OpenMP constructs may not
>> be nested inside a simd region}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp for // expected-error {{region cannot be closely nested
>> inside 'for' region; perhaps you forget to enclose 'omp for'
>> directive into a parallel region?}}
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp simd
>> + for (int i = 0; i < 10; ++i);
>> + }
>> +#pragma omp for
>> + for (int i = 0; i < 10; ++i) {
>> +#pragma omp parallel
>> + for (int i = 0; i < 10; ++i);
>> + }
>> + return foo<int>();
>> +}
>> +
>>
>> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp
>>
> ------------------------------------------------------------------------------
>
>> svn:eol-style = native
>>
>> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp
>>
> ------------------------------------------------------------------------------
>
>> svn:keywords = Author Date Id Rev URL
>>
>> Propchange: cfe/trunk/test/OpenMP/nesting_of_regions.cpp
>>
> ------------------------------------------------------------------------------
>
>> svn:mime-type = text/plain
>>
>>
>>
>>
>> ------------------------------
>>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140626/0739f9d2/attachment.html>
>
> ------------------------------
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> End of cfe-commits Digest, Vol 84, Issue 504
> ********************************************
More information about the cfe-commits
mailing list