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