r211566 - [OPENMP] Added initial checking of nesting of OpenMP regions.

Alexey Bataev a.bataev at hotmail.com
Thu Jun 26 21:55:01 PDT 2014


> > > 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?
>
> I meant some sort of static table whose elements contain the valid 
> nestings, like:
>
> [Parent Directive Kind, Current Directive Kind, Closely|No-Closely|Both ]
Ok, got it. I'll add this table as a comment to the function.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team

27.06.2014 8:50, Samuel F Antao пишет:
>
> Hi Alexey,
>
> Alexey Bataev <a.bataev at hotmail.com> wrote on 06/26/2014 11:29:04 PM:
>
> > From: Alexey Bataev <a.bataev at hotmail.com>
> > To: cfe-commits at cs.uiuc.edu, Samuel F Antao/Watson/IBM at IBMUS
> > Date: 06/26/2014 11:29 PM
> > Subject: Re: r211566 - [OPENMP] Added initial checking of nesting of
> > OpenMP regions.
> >
> > 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.
>
> Ok. Makes sense.
>
> > > 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?
>
> I meant some sort of static table whose elements contain the valid 
> nestings, like:
>
> [Parent Directive Kind, Current Directive Kind, Closely|No-Closely|Both ]
>
> If the nesting under test is not in the table it would fail. As I have 
> mentioned, the code looks okay
> right now, but in the future if we see that function starts to get 
> messy due to the nestings combinations
> one can implement a solution like this.
>
> > > 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.
>
> Ok, thanks.
>
> > > 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.
>
> Got it.
>
> >
> > 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
> > > ********************************************
> >
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140627/6f7031e9/attachment.html>


More information about the cfe-commits mailing list