[PATCH] [OPENMP] Codegen for 'if' clause

[John McCall]

With these changes (and Hal's), looks good to me.

================
Comment at: include/clang/AST/StmtOpenMP.h:132
@@ +131,3 @@
+  /// \brief Gets single clause of the specified kind \a K associated with the
+  /// current directive iff there is only one clause of this kind.
+  const OMPClause *getSingleClause(OpenMPClauseKind K) const;
----------------
hfinkel wrote:
> Make sure this gets the improved comment text from D5145.
This comment should clarify that it returns nil if there isn't a clause of that kind.  It only asserts that there aren't multiple clauses of that kind.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:134
@@ +133,3 @@
+  llvm::Value *ThreadID = nullptr;
+  OpenMPThreadIDMapTy::iterator I = OpenMPThreadIDMap.find(CGF.CurFn);
+  if (I != OpenMPThreadIDMap.end()) {
----------------
It'd be nice to leave a comment here like "Check whether we've already cached a load of the thread id in this function."

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:145
@@ +144,3 @@
+    ThreadID = CGF.EmitLoadOfLValue(LVal, SourceLocation()).getScalarVal();
+    // If value loaded in entry block, use it everywhere in function.
+    if (CGF.Builder.GetInsertBlock() == CGF.AllocaInsertPt->getParent())
----------------
Is there a reason we can't safely always load in the entry block?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:149
@@ -130,27 +148,3 @@
   } else {
-    // Check if current function is a function which has first parameter
-    // with type int32 and name ".global_tid.".
-    if (!CGF.CurFn->arg_empty() &&
-        CGF.CurFn->arg_begin()->getType()->isPointerTy() &&
-        CGF.CurFn->arg_begin()
-            ->getType()
-            ->getPointerElementType()
-            ->isIntegerTy() &&
-        CGF.CurFn->arg_begin()
-                ->getType()
-                ->getPointerElementType()
-                ->getIntegerBitWidth() == 32 &&
-        CGF.CurFn->arg_begin()->hasName() &&
-        CGF.CurFn->arg_begin()->getName() == ".global_tid.") {
-      CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
-      CGF.Builder.SetInsertPoint(CGF.AllocaInsertPt);
-      GTid = CGF.Builder.CreateLoad(CGF.CurFn->arg_begin());
-    } else {
-      // Generate "int32 .kmpc_global_thread_num.addr;"
-      CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
-      CGF.Builder.SetInsertPoint(CGF.AllocaInsertPt);
-      llvm::Value *Args[] = {EmitOpenMPUpdateLocation(CGF, Loc)};
-      GTid = CGF.EmitRuntimeCall(
-          CreateRuntimeFunction(OMPRTL__kmpc_global_thread_num), Args);
-    }
-    OpenMPGtidMap[CGF.CurFn] = GTid;
+    // Generate "int32 .kmpc_global_thread_num.addr;"
+    CGBuilderTy::InsertPointGuard IPG(CGF.Builder);
----------------
This comment doesn't seem be accurate.  You're actually generating a call.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:161
@@ -161,2 +160,3 @@
 void CGOpenMPRuntime::FunctionFinished(CodeGenFunction &CGF) {
   assert(CGF.CurFn && "No function in current CodeGenFunction.");
+  if (OpenMPThreadIDMap.count(CGF.CurFn))
----------------
Would it make more sense for the CodeGenFunction to just have a lazily-allocated cache of arbitrary information associated with the OpenMPRuntime, rather than having tons of extra side-tables?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:241
@@ +240,3 @@
+  // Build calls:
+  // __kmpc_serialized_parallel(&Loc, GTid);
+  llvm::Value *SerArgs[] = {EmitOpenMPUpdateLocation(CGF, Loc), ThreadID};
----------------
Please leave a blank line before the emission of each call so that these don't all run together.

<-  That is, put one here.  (Before the comment line.)

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:246
@@ +245,3 @@
+  CGF.EmitRuntimeCall(RTLFn, SerArgs);
+  // OutlinedFn(&GTid, &zero, CapturedStruct);
+  auto ThreadIDAddr = EmitThreadIDAddress(CGF, Loc);
----------------
<- And here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:254
@@ +253,3 @@
+  CGF.EmitCallOrInvoke(OutlinedFn, OutlinedFnArgs);
+  // __kmpc_end_serialized_parallel(&Loc, GTid);
+  llvm::Value *EndSerArgs[] = {EmitOpenMPUpdateLocation(CGF, Loc), ThreadID};
----------------
<- And here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:37
@@ +36,3 @@
+  /// the thread executing OpenMP construct. The type of this variable is
+  /// kmp_int32.
+  const VarDecl *getThreadIDVariable() const { return ThreadIDVar; }
----------------
We don't necessarily need to do this right now, but eventually I think it makes sense to abstract this type so that it's private to the target runtime.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:160
@@ +159,3 @@
+  /// \brief Gets thread id value for the current thread.
+  /// \param CGF Reference to current CodeGenFunction.
+  /// \param Loc Clang source location.
----------------
You don't need to comment CGF parameters; they're everywhere, and their purpose is obvious.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:161
@@ -144,1 +160,3 @@
+  /// \param CGF Reference to current CodeGenFunction.
+  /// \param Loc Clang source location.
   ///
----------------
This kind of comment doesn't add anything that's not expressed in the type of the parameter.

A SourceLocation parameter to an Emit function has a conventional meaning in CodeGen, which is "here's a location to associate with this operation, for debug locations or whatever else".  You don't need to comment such parameters unless the SourceLocation's going to be used for some other purpose.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:192
@@ -158,1 +191,3 @@
   /// \param Loc Clang source location.
+  /// \param OutlinedFn Outlined function to be run in parallel threads.
+  /// \param CapturedStruct A pointer to the record with the references to
----------------
Please describe the expected type of the value.  It's something like void(*)(kmp_int32, struct context_vars*), right?

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:26
@@ -25,1 +25,3 @@
 
+static void EmitOMPIfClause(CodeGenFunction &CGF, const Expr *Cond,
+                            std::function<void(bool)> CodeGen) {
----------------
This deserves a comment, if just to explain what the parameter to the CodeGen callback means.

Also, please take the std::function as a const &.

http://reviews.llvm.org/D4716