[PATCH] [OPENMP] Codegen for threadprivate variables

[John McCall]

Okay, the big problem here is that the AST representation for threadprivate does not make any sense.  This absolutely needs to be recorded on the target declaration itself somehow.  I'm sorry this wasn't caught during the patch which added the Sema work.

I would suggest doing this by adding an implicit attribute to the decl.  In order to work correctly with PCH/modules, you will potentially need to serialize what's called an "update record" to ensure that chained PCH files which add threadprivate to an existing var from a previous module include the added attribute.

This should also eliminate the need for these "is this a threadprivate variable" side tables.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:217
@@ +216,3 @@
+                                /*isVarArg*/ false)->getPointerTo();
+    // typedef void (*kmpc_dtor)(void *);
+    llvm::Type *KmpcCCtorTyArgs[] = {CGM.VoidPtrTy, CGM.VoidPtrTy};
----------------
You have the comments backwards here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:219
@@ +218,3 @@
+    llvm::Type *KmpcCCtorTyArgs[] = {CGM.VoidPtrTy, CGM.VoidPtrTy};
+    auto KmpcCCtorTy =
+        llvm::FunctionType::get(CGM.VoidPtrTy, KmpcCCtorTyArgs,
----------------
Please don't make separate variables called kmpcCtorTy and kmpcCCtorTy.  kmpcCopyCtorTy, maybe?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:278
@@ +277,3 @@
+
+llvm::Constant *
+CGOpenMPRuntime::getOrCreateThreadPrivateCache(const VarDecl *VD,
----------------
Please document the expectations about this cache variable.  It's enough to say that it's a pointer's worth of storage that's reserved for use by the OpenMP runtime.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:281
@@ +280,3 @@
+                                               llvm::Value *Addr) {
+  StringRef MangledName = CGM.getMangledName(VD);
+  // Lookup the entry, lazily creating it if necessary.
----------------
Why is this lookup by name, rather than by the canonical decl pointer?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:288
@@ +287,3 @@
+        CGM.getModule(), CGM.Int8PtrPtrTy, /*IsConstant*/ false,
+        llvm::GlobalValue::CommonLinkage,
+        llvm::Constant::getNullValue(CGM.Int8PtrPtrTy), ".cache.",
----------------
Don't create a variable with non-internal linkage and a meaningless name.  Two different translation units can use the same meaningless name for the caches for different variables.

If you're going to stick with a meaningless name, you need to use internal linkage for the cache variable.

Alternatively, if you use a meaningful name (perhaps based on the mangled name of the cached variable?), you can potentially share caches across translation units.  The optimal sharing strategy depends on the linkage of the original global variable: if it's internal, you should use internal linkage with default visibility (because the cache must not be shared with a variable from a different file), and otherwise you should use linkonce_odr linkage and hidden visibility (so that the cache will be shared as broadly as possible without introducing unnecessary work for the dynamic linker).

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:310
@@ +309,3 @@
+
+llvm::Constant *CGOpenMPRuntime::EmitOMPCXXThreadPrivateInitFunction(
+    const VarDecl &VD, llvm::Constant *Addr, bool PerformInit,
----------------
Comment your parameters here, please.  Especially Addr.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:312
@@ +311,3 @@
+    const VarDecl &VD, llvm::Constant *Addr, bool PerformInit,
+    bool PerformDestroy, SourceLocation Loc) {
+  QualType ASTTy = VD.getType();
----------------
You should pass in the result of isDestructedType() instead of repeatedly recomputing it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:316
@@ +315,3 @@
+  llvm::Value *Ctor = nullptr, *CCtor = nullptr, *Dtor = nullptr;
+  if (PerformInit) {
+    CodeGenFunction CtorCGF(CGM);
----------------
Great place for a comment explaining what each of these functions does.  For example, this function re-emits the declaration's initializer into the thread-local copy of the variable.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:331
@@ +330,3 @@
+                          Args, SourceLocation());
+    auto Arg = CtorCGF.EmitScalarConversion(
+        Fn->arg_begin(), CGM.getContext().VoidPtrTy,
----------------
Please just do a bitcast here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:341
@@ +340,3 @@
+  }
+  if (PerformDestroy) {
+    CodeGenFunction DtorCGF(CGM);
----------------
Document the purpose of this function, please.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:1413
@@ +1412,3 @@
+  if (const auto *VD = dyn_cast<VarDecl>(D)) {
+    EmitGlobalVarDefinition(VD);
+    if (getLangOpts().OpenMP && getOpenMPRuntime().isOMPThreadPrivateDecl(VD))
----------------
hfinkel wrote:
> We should add a comment here explaining the separation of concerns between what EmitGlobalVarDefinition does, what EmitOMPThreadPrivateVarDecl does, and why they're both called here).
I would go so far as to say that EmitGlobalVarDefinition should implicitly handle the threadprivate stuff itself whenever possible.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:3400
@@ +3399,3 @@
+    CXXRecordDecl *RD = ASTTy->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
+    bool PerformInit = RD && VD->getAnyInitializer() != nullptr;
+    bool PerformDestroy = RD && !RD->hasTrivialDestructor();
----------------
In C++, you can have non-trivial initializers on global variables of arbitrary type, like so:
  void *buffer = malloc(1024);

So you may need to PerformInit regardless of whether RD is null.

================
Comment at: lib/CodeGen/CodeGenModule.cpp:3401
@@ +3400,3 @@
+    bool PerformInit = RD && VD->getAnyInitializer() != nullptr;
+    bool PerformDestroy = RD && !RD->hasTrivialDestructor();
+    auto DeclPtr = VD->isStaticLocal() ? getStaticLocalDeclAddress(VD)
----------------
Use isDestructedType().

================
Comment at: lib/CodeGen/CodeGenModule.cpp:3405
@@ +3404,3 @@
+
+    if (DeclPtr && (PerformInit || PerformDestroy)) {
+      auto InitFunction =
----------------
It is really unfortunate that you need an eager global initializer to register a static local variable.  Please change this so that it's only registered the first time that the variable declaration is "evaluated", within the initializer guards.

That should be straightforward: we'll never emit a static local variable before we've fully processed the function body (and hence seen any openmp pragmas lurking within).

================
Comment at: lib/CodeGen/ModuleBuilder.cpp:132
@@ -130,1 +131,3 @@
       Builder->UpdateCompletedType(D);
+      // In C++, we may have member threadprivate decl that need to be emitted
+      // at this point.
----------------
Okay, at the very least, this should be guarded by the OpenMP language option.  But I think it would be better to handle this directly in EmitGlobalVarDefinition, when a var is already known to be threadprivate during its emission, which should include all C++ static member cases.  It's only true global-scoped variables where CodeGen might process the definition before knowing that it's supposed to be threadprivate.

================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2202
@@ -2201,3 +2201,3 @@
       isa<ObjCImplDecl>(D) ||
-      isa<ImportDecl>(D))
+      isa<ImportDecl>(D) || isa<OMPThreadPrivateDecl>(D))
     return true;
----------------
Please follow the pattern of putting each decl kind on its own line.

http://reviews.llvm.org/D4002