[PATCH] [OPENMP] Bug fixes in threadprivate declaration and data sharing attributes processing.

[Hal Finkel]

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" <a.bataev at hotmail.com>
> To: dgregor at apple.com, hfinkel at anl.gov, cbergstrom at pathscale.com, "a bataev" <a.bataev at hotmail.com>
> Cc: cfe-commits at cs.uiuc.edu
> 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