[PATCH] [OPENMP] Codegen for threadprivate variables

[John McCall]

Looks great!  A few comments inline, and then I think it's ready to go.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:374
@@ +373,3 @@
+  return GetOrCreateInternalVariable(
+      CGM.Int8PtrPtrTy, Twine(CGM.getMangledName(VD), ".cache."));
+}
----------------
Please use
  Twine(...) + ".cache."
instead of calling the ctor directly.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:421
@@ +420,3 @@
+        !Init->isConstantInitializer(CGM.getContext(),
+                                     /*ForRef=*/false)) {
+      // Generate function that re-emits the declaration's initializer into the
----------------
Can we avoid having to recompute isConstantInitializer? We should know this from context.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:170
@@ -160,2 +169,3 @@
+      OMPInternalVarNames;
 
   /// \brief Emits object of ident_t type with info for source location.
----------------
Comment: it's a map from the unique names to the variables, not the other way around.  Along those same lines, please name it something like InternalVars; the names are the keys, and OMP is implicit from context.

Using a StringMap here isn't the most efficient thing when you're hashing on a place where you have a declaration, but it's okay, and if you think it's worthwhile to reuse the same StringMap for different uses this way, fine.  (But please document all the different things that get put in it!)

Instead of holding a naked llvm::GlobalVariable*, though, please use an llvm::AssertingVH<llvm::Constant>, which will correctly be updated if something needs to replaceAllUsesWith the original global variable you created.

================
Comment at: lib/Sema/SemaOpenMP.cpp:852
@@ -851,1 +851,3 @@
+    VD->addAttr(OMPThreadPrivateDeclAttr::CreateImplicit(
+        Context, SourceRange(Loc, Loc)));
   }
----------------
If you want to get PCH / modules correct when the global decl has been separated from the threadprivate pragma (which might be an unnecessary corner case — up to you), you'll need to add a update record.

The way you do that is to:
1. check to see if the decl you're modifying came from an AST file, and if so:
2. add a new virtual method to ASTMutationListener,
3. implement it in ASTWriter by adding an update record to the AST, and then
4. handle that update record in ASTReader by adding the attribute retroactively (to all subsequent redeclarations).

http://reviews.llvm.org/D4002