[PATCH] [OPENMP] Fixed codegen for arrays in 'copyprivate' clause.

[John McCall]

Generally looks good.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:991
@@ -986,1 +990,3 @@
+             const VarDecl * /*DestVD*/, const VarDecl * /*SrcVD*/,
+             const Expr * /*AssignOp*/)> &SingleCopyprivateCodeGen) {
   auto &C = CGM.getContext();
----------------
Same.  Although maybe you should just have a typedef for this?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:362
@@ -360,1 +361,3 @@
+               const VarDecl * /*DestVD*/, const VarDecl * /*SrcVD*/,
+               const Expr * /*AssignOp*/)> &SingleCopyprivateCodeGen);
 
----------------
You can just leave these parameter names there without commenting them out.

Should SingleOpGen also be a llvm::function_ref?

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:117
@@ +116,3 @@
+
+  // Shift the address back by one element.
+  auto NegativeOne = llvm::ConstantInt::get(SizeTy, -1, true);
----------------
Is there a reason why this is back-to-front instead of front-to-back?  In C++, arrays are generally initialized front-to-back, so that when destruction goes back-to-front it happens in reverse order of construction.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:162
@@ +161,3 @@
+          auto *AssignExpr = VD->getInit();
+          if (!isa<CXXConstructExpr>(AssignExpr) ||
+              isTrivialInitializer(AssignExpr)) {
----------------
If this is an initialization expression, please don't call it an assignment expression.  If it's an assignment expression, checking for CXXConstructExpr is not going to work.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:173
@@ +172,3 @@
+                                           llvm::Value *SrcElement) {
+                  // Create RunCleanScope to cleanup possible temps.
+                  RunCleanupsScope Init(*this);
----------------
It's called "RunCleanupsScope", but that part of the comment doesn't add much anyway.  I'd say something like "Clean up any temporaries needed by the assignment expression."

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:973
@@ +972,3 @@
+      *this,
+      [&]() -> void {
+        InlinedOpenMPRegionScopeRAII Region(*this, S);
----------------
You don't need the "()->void", so unless you find it helpful, it's best to leave it off.

http://reviews.llvm.org/D8914

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/