r211566 - [OPENMP] Added initial checking of nesting of OpenMP regions.
Samuel F Antao
sfantao at us.ibm.com
Thu Jun 26 21:50:11 PDT 2014
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/26058f39/attachment.html>
More information about the cfe-commits
mailing list