Alexey, I think this is fine to commit (and, generally speaking, we can do post-commit reviews for small bug fixes like this).
My general inclination is to use llvm::next and llvm::prior instead of writing Stack.rbegin() + 1 and Stack.rend() - 1, but we already have a number of these +-1 expressions on the DSA stack iterators, and we should keep everything consistent (and maybe change them all in a separate commit). Thanks again, Hal ----- Original Message ----- > From: "Alexey Bataev" <[email protected]> > To: [email protected], [email protected], [email protected], "a bataev" > <[email protected]> > Cc: [email protected] > Sent: Thursday, December 19, 2013 10:24:33 PM > Subject: [PATCH] [OPENMP] Bug fixes in threadprivate declaration and data > sharing attributes processing. > > Hi doug.gregor, hfinkel, cbergstrom, > > http://llvm-reviews.chandlerc.com/D2451 > > Files: > test/OpenMP/threadprivate_ast_print.cpp > lib/Sema/SemaOpenMP.cpp > > Index: test/OpenMP/threadprivate_ast_print.cpp > =================================================================== > --- test/OpenMP/threadprivate_ast_print.cpp > +++ test/OpenMP/threadprivate_ast_print.cpp > @@ -2,8 +2,6 @@ > // RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -emit-pch -o %t %s > // RUN: %clang_cc1 -fopenmp -std=c++11 -include-pch %t -fsyntax-only > -verify %s -ast-print > // expected-no-diagnostics > -// FIXME: This test has been crashing since r186647. > -// REQUIRES: disabled > > #ifndef HEADER > #define HEADER > Index: lib/Sema/SemaOpenMP.cpp > =================================================================== > --- lib/Sema/SemaOpenMP.cpp > +++ lib/Sema/SemaOpenMP.cpp > @@ -82,6 +82,9 @@ > typedef SmallVector<SharingMapTy, 8>::reverse_iterator > reverse_iterator; > > DSAVarData getDSA(StackTy::reverse_iterator Iter, VarDecl *D); > + > + /// \brief Checks if the variable is a local for OpenMP region. > + bool isOpenMPLocal(VarDecl *D, StackTy::reverse_iterator Iter); > public: > explicit DSAStackTy(Sema &S) : Stack(1), Actions(S) { } > > @@ -98,9 +101,6 @@ > /// \brief Adds explicit data sharing attribute to the specified > declaration. > void addDSA(VarDecl *D, DeclRefExpr *E, OpenMPClauseKind A); > > - /// \brief Checks if the variable is a local for OpenMP region. > - bool isOpenMPLocal(VarDecl *D); > - > /// \brief Returns data sharing attributes from top of the stack > for the > /// specified declaration. > DSAVarData getTopDSA(VarDecl *D); > @@ -152,7 +152,21 @@ > > return DVar; > } > + > DVar.DKind = Iter->Directive; > + // OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables > Referenced > + // in a Construct, C/C++, predetermined, p.1] > + // Variables with automatic storage duration that are declared in > a scope > + // inside the construct are private. > + if (DVar.DKind != OMPD_parallel) { > + if (isOpenMPLocal(D, Iter) && D->isLocalVarDecl() && > + (D->getStorageClass() == SC_Auto || > + D->getStorageClass() == SC_None)) { > + DVar.CKind = OMPC_private; > + return DVar; > + } > + } > + > // Explicitly specified attributes and local variables with > predetermined > // attributes. > if (Iter->SharingMap.count(D)) { > @@ -231,23 +245,23 @@ > } > } > > -bool DSAStackTy::isOpenMPLocal(VarDecl *D) { > - Scope *CurScope = getCurScope(); > - while (CurScope && !CurScope->isDeclScope(D)) > - CurScope = CurScope->getParent(); > - while (CurScope && !CurScope->isOpenMPDirectiveScope()) > - CurScope = CurScope->getParent(); > - bool isOpenMPLocal = !!CurScope; > - if (!isOpenMPLocal) { > - CurScope = getCurScope(); > - while (CurScope && !CurScope->isOpenMPDirectiveScope()) > +bool DSAStackTy::isOpenMPLocal(VarDecl *D, StackTy::reverse_iterator > Iter) { > + if (Stack.size() > 2) { > + reverse_iterator I = Iter, E = Stack.rend() - 1; > + Scope *TopScope = 0; > + while (I != E && > + I->Directive != OMPD_parallel) { > + ++I; > + } > + if (I == E) return false; > + TopScope = I->CurScope ? I->CurScope->getParent() : 0; > + Scope *CurScope = getCurScope(); > + while (CurScope != TopScope && !CurScope->isDeclScope(D)) { > CurScope = CurScope->getParent(); > - isOpenMPLocal = > - CurScope && > - isa<CapturedDecl>(D->getDeclContext()) && > - > CurScope->getFnParent()->getEntity()->Encloses(D->getDeclContext()); > + } > + return CurScope != TopScope; > } > - return isOpenMPLocal; > + return false; > } > > DSAStackTy::DSAVarData DSAStackTy::getTopDSA(VarDecl *D) { > @@ -270,11 +284,13 @@ > // in a Construct, C/C++, predetermined, p.1] > // Variables with automatic storage duration that are declared in > a scope > // inside the construct are private. > - if (isOpenMPLocal(D) && D->isLocalVarDecl() && > - (D->getStorageClass() == SC_Auto || > - D->getStorageClass() == SC_None)) { > - DVar.CKind = OMPC_private; > - return DVar; > + OpenMPDirectiveKind Kind = getCurrentDirective(); > + if (Kind != OMPD_parallel) { > + if (isOpenMPLocal(D, Stack.rbegin() + 1) && D->isLocalVarDecl() > && > + (D->getStorageClass() == SC_Auto || > + D->getStorageClass() == SC_None)) > + DVar.CKind = OMPC_private; > + return DVar; > } > > // OpenMP [2.9.1.1, Data-sharing Attribute Rules for Variables > Referenced > @@ -319,7 +335,7 @@ > // in a Construct, C/C++, predetermined, p.7] > // Variables with static storage duration that are declared in a > scope > // inside the construct are shared. > - if (isOpenMPLocal(D) && D->isStaticLocal()) { > + if (D->isStaticLocal()) { > DVar.CKind = OMPC_shared; > return DVar; > } > @@ -567,10 +583,13 @@ > > Vars.push_back(*I); > } > - return Vars.empty() ? > - 0 : OMPThreadPrivateDecl::Create(Context, > - > getCurLexicalContext(), > - Loc, Vars); > + OMPThreadPrivateDecl *D = 0; > + if (!Vars.empty()) { > + D = OMPThreadPrivateDecl::Create(Context, > getCurLexicalContext(), Loc, > + Vars); > + D->setAccess(AS_public); > + } > + return D; > } > > namespace { > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
