r211566 - [OPENMP] Added initial checking of nesting of OpenMP regions.
Samuel F Antao
sfantao at us.ibm.com
Thu Jun 26 15:48:56 PDT 2014
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.llvm.org/pipermail/cfe-commits/attachments/20140626/0739f9d2/attachment.html>
More information about the cfe-commits
mailing list