[PATCH] [OPENMP] Codegen for threadprivate variables

[Alexey Bataev]

Hal, John, Thanks for the review!
I'll get back to you soon with the updated patch.

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

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

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:278
@@ +277,3 @@
+
+llvm::Constant *
+CGOpenMPRuntime::getOrCreateThreadPrivateCache(const VarDecl *VD,
----------------
rjmccall wrote:
> 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.
Ok

================
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.
----------------
rjmccall wrote:
> Why is this lookup by name, rather than by the canonical decl pointer?
VarDecl is the redeclarable construct and it may have several instances. MangledName is the same for all redeclarations. I'll try to improve it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:288
@@ +287,3 @@
+        CGM.getModule(), CGM.Int8PtrPtrTy, /*IsConstant*/ false,
+        llvm::GlobalValue::CommonLinkage,
+        llvm::Constant::getNullValue(CGM.Int8PtrPtrTy), ".cache.",
----------------
rjmccall wrote:
> 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).
Agree, my fault. Earlier there was meaningful name, but it was removed somehow during previous updates.

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

================
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();
----------------
rjmccall wrote:
> You should pass in the result of isDestructedType() instead of repeatedly recomputing it.
Agree. I'll rework this

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:316
@@ +315,3 @@
+  llvm::Value *Ctor = nullptr, *CCtor = nullptr, *Dtor = nullptr;
+  if (PerformInit) {
+    CodeGenFunction CtorCGF(CGM);
----------------
rjmccall wrote:
> 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.
Agree, I'll add comments

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

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

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:364
@@ +363,3 @@
+                                         /*isVarArg*/ false)->getPointerTo();
+  CCtor = llvm::Constant::getNullValue(CCtorTy);
+  if (Ctor == nullptr) {
----------------
rjmccall wrote:
> As elsewhere, please rename CCtor to be more easily distinguishable from Ctor.
> 
> Please also document what the purpose of this function is and why it's okay to always pass null here.
Ok

================
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))
----------------
rjmccall wrote:
> 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.
Ok, I'll rework this. I'll add an attribute for thread private variables to handle it in GlobalVarDefinitions.

================
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();
----------------
rjmccall wrote:
> 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.
I'll fix this

================
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)
----------------
rjmccall wrote:
> Use isDestructedType().
Ok

================
Comment at: lib/CodeGen/CodeGenModule.cpp:3405
@@ +3404,3 @@
+
+    if (DeclPtr && (PerformInit || PerformDestroy)) {
+      auto InitFunction =
----------------
rjmccall wrote:
> 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).
Agree, I'll try to fix this.

================
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.
----------------
rjmccall wrote:
> 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.
Ok

================
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;
----------------
rjmccall wrote:
> Please follow the pattern of putting each decl kind on its own line.
Ok

http://reviews.llvm.org/D4002