r209716 - [OPENMP] Additional checking for local vars in initial values for threadprivate vars

Michael Wong fraggamuffin at gmail.com
Thu Jun 5 12:26:38 PDT 2014


Hi, nice patch. Fairly simple, just adding a check for the following case.
This use case, that of initializing a static class with a local variable
and then making it threadprivate has always been problematic in OpenMP
(i.e. not clearly defined although I think where you are going is right):
static Class LocalClass(y); // expected-error {{variable with local storage
in initial value of threadprivate variable}}
#pragma omp threadprivate(LocalClass)
 My comment is more about this situation. We had a call discussion on this
amoong the OpenMP clang review group (weekly). Ajay Jayaraj of TI started
the thought that when Threadprivate Static vars that are initialized with
initial local values, the precise method of initialization is ambiguously
defined by OpenMP. Hal Finkel added that there has always been question in
the OpenMP spec on when do you call the constructor. Normally initializing
a static variable is done with an atomic flag or some form of double
checked locking on the static var when you are in a parallel context to
ensure a once only initialization (similarly for a functional local
static). Here we also need to have it initialized once for every thread
(threadprivate) and have it last the duration of the program. There is now
question as to when this should happen (any time before first use) and what
constructor is called for each thread, likely the Default constructor.

This is one of those C++'ism that OpenMP fails to fully specify and it
should be brought back to the OpenMP committee as a defect. I have been
collecting these cases where OpenMP does not serve C++ particularly well,
and this is one of several that we can improve upon.

One other very nick-picky comment. The test cases all use Class as a class
to hold an abstraction for  a test class. Even as a C++ Std committee
member, I find having a user name entity that uses the same name as a C++
abstraction  (class) confusing. I mean, was this deliberate (it was), or
was this an accidental typo that somehow worked? Why not stay far away from
that kind of doubt and umambiguously call it TestClass or TempClass as you
have done with another class. Your choice.


On Wed, May 28, 2014 at 3:40 AM, Alexey Bataev <a.bataev at hotmail.com> wrote:

> Author: abataev
> Date: Wed May 28 02:40:25 2014
> New Revision: 209716
>
> URL: http://llvm.org/viewvc/llvm-project?rev=209716&view=rev
> Log:
> [OPENMP] Additional checking for local vars in initial values for
> threadprivate vars
>
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>     cfe/trunk/lib/Sema/SemaOpenMP.cpp
>     cfe/trunk/test/OpenMP/threadprivate_messages.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=209716&r1=209715&r2=209716&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed May 28
> 02:40:25 2014
> @@ -6956,6 +6956,8 @@ def err_omp_linear_expected_int_or_ptr :
>  def warn_omp_linear_step_zero : Warning<
>    "zero linear step (%0 %select{|and other variables in clause }1should
> probably be const)">,
>    InGroup<OpenMPClauses>;
> +def err_omp_local_var_in_threadprivate_init : Error<
> +  "variable with local storage in initial value of threadprivate
> variable">;
>  } // 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=209716&r1=209715&r2=209716&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Wed May 28 02:40:25 2014
> @@ -553,6 +553,35 @@ Sema::ActOnOpenMPThreadprivateDirective(
>    return DeclGroupPtrTy();
>  }
>
> +namespace {
> +class LocalVarRefChecker : public ConstStmtVisitor<LocalVarRefChecker,
> bool> {
> +  Sema &SemaRef;
> +
> +public:
> +  bool VisitDeclRefExpr(const DeclRefExpr *E) {
> +    if (auto VD = dyn_cast<VarDecl>(E->getDecl())) {
> +      if (VD->hasLocalStorage()) {
> +        SemaRef.Diag(E->getLocStart(),
> +                     diag::err_omp_local_var_in_threadprivate_init)
> +            << E->getSourceRange();
> +        SemaRef.Diag(VD->getLocation(), diag::note_defined_here)
> +            << VD << VD->getSourceRange();
> +        return true;
> +      }
> +    }
> +    return false;
> +  }
> +  bool VisitStmt(const Stmt *S) {
> +    for (auto Child : S->children()) {
> +      if (Child && Visit(Child))
> +        return true;
> +    }
> +    return false;
> +  }
> +  LocalVarRefChecker(Sema &SemaRef) : SemaRef(SemaRef) {}
> +};
> +} // namespace
> +
>  OMPThreadPrivateDecl *
>  Sema::CheckOMPThreadPrivateDecl(SourceLocation Loc, ArrayRef<Expr *>
> VarList) {
>    SmallVector<Expr *, 8> Vars;
> @@ -592,6 +621,13 @@ Sema::CheckOMPThreadPrivateDecl(SourceLo
>        continue;
>      }
>
> +    // Check if initial value of threadprivate variable reference
> variable with
> +    // local storage (it is not supported by runtime).
> +    if (auto Init = VD->getAnyInitializer()) {
> +      LocalVarRefChecker Checker(*this);
> +      if (Checker.Visit(Init)) continue;
> +    }
> +
>      Vars.push_back(RefExpr);
>      DSAStack->addDSA(VD, DE, OMPC_threadprivate);
>    }
>
> Modified: cfe/trunk/test/OpenMP/threadprivate_messages.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/threadprivate_messages.cpp?rev=209716&r1=209715&r2=209716&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/OpenMP/threadprivate_messages.cpp (original)
> +++ cfe/trunk/test/OpenMP/threadprivate_messages.cpp Wed May 28 02:40:25
> 2014
> @@ -108,10 +108,12 @@ int o; // expected-note {{candidate foun
>
>  int main(int argc, char **argv) { // expected-note {{'argc' defined here}}
>
> -  int x, y = argc; // expected-note {{'y' defined here}}
> +  int x, y = argc; // expected-note 2 {{'y' defined here}}
>    static double d1;
>    static double d2;
>    static double d3; // expected-note {{'d3' defined here}}
> +  static Class LocalClass(y); // expected-error {{variable with local
> storage in initial value of threadprivate variable}}
> +#pragma omp threadprivate(LocalClass)
>
>    d.a = a;
>    d2++;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140605/aa00dfb6/attachment.html>


More information about the cfe-commits mailing list