[PATCH] [OPENMP] Initial codegen for 'omp task' directive.

[John McCall]

Sorry for the delay.  Phabricator's been acting up; you'll notice a couple of comments that I wasn't able to delete, because deleting them locked the webpage up and never applied the deletion.  Hooray for tools.

Generally looking better.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:44
@@ -43,3 +43,3 @@
   /// \brief Get an LValue for the current ThreadID variable.
-  LValue getThreadIDVariableLValue(CodeGenFunction &CGF);
+  virtual LValue getThreadIDVariableLValue(CodeGenFunction &CGF);
 
----------------
Okay.  I think I see what you've done here with the subclass, so that this always produces a ident_t l-value, and thus its address is always an ident_t*.  That makes a lot of sense to me.  Please document that.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:91
@@ +90,3 @@
+  /// \brief Get a variable or parameter for storing global thread id
+  /// inside OpenMP construct.
+  virtual const VarDecl *getThreadIDVariable() const override {
----------------
.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:170
@@ -130,1 +169,3 @@
+          ->castAs<PointerType>()
+          ->getPointeeType());
 }
----------------
.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:171
@@ -130,2 +170,3 @@
+          ->getPointeeType());
 }
 
----------------
.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:229
@@ +228,3 @@
+  assert(!ThreadIDVar->getType()->isPointerType() &&
+         "thread id variable must be of type kmp_int32 for tasks");
+  auto *CS = cast<CapturedStmt>(D.getAssociatedStmt());
----------------
This is a great assertion, thanks.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1138
@@ +1137,3 @@
+namespace {
+/// \brief Fields for type kmp_task_t.
+enum KmpTaskTFields {
----------------
*Indexes* of fields in type kmp_task_t.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1200
@@ +1199,3 @@
+  auto KmpInt32Ty = C.getIntTypeForBitwidth(/*DestWidth=*/32, /*Signed=*/1);
+  // Build type kmp_routine_entry_t (if not built yet.
+  if (!KmpRoutineEntryPtrTy) {
----------------
Closing parenthesis.  Also, I'd make a getter for this instead of inlining it here.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1230
@@ +1229,3 @@
+  auto *TaskEntry = llvm::Function::Create(
+      TaskEntryTy, /*Linkage=*/llvm::GlobalValue::InternalLinkage,
+      ".omp_task_entry.", &CGM.getModule());
----------------
Argument comments are useful when somebody reading the code might not understand the purpose of the argument.  In this case, that purpose is pretty obvious from the argument expression itself, so the comment is useless.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1266
@@ +1265,3 @@
+                                              KmpInt32Ty));
+  TaskEntryCGF.FinishFunction();
+
----------------
It's generally better to emit helper functions in a separate function, so that any particular function only knows about one CodeGenFunction object:
 - It removes some code from what's already a pretty large function.
 - It makes the shared data between the two functions a lot more obvious.
 - It removes any confusion about which function any particular llvm::Value* is valid in, so that you aren't tempted to (e.g.) use SharedsParam in the CGF function instead of the TaskEntryCGF function.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1302
@@ +1301,3 @@
+            /*Volatile=*/false, CGM.PointerAlignInBytes, SharedsPtrTy, Loc),
+        Shareds, SharedsTy);
+  // TODO: generate function with destructors for privates.
----------------
You're probably going to need to ask Sema to generate a copy constructor expression to get this right in C++.  Feel free to do that in a separate patch.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1318
@@ +1317,3 @@
+                             getThreadID(CGF, Loc), NewTask};
+  CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__kmpc_omp_task), TaskArgs);
+}
----------------
Nothing cares about the result of this call?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:273
@@ -264,1 +272,3 @@
+  /// This outlined function has type void(*)(kmp_int32 */*ThreadID*/, kmp_int32
+  /// /*BoundID*/, struct context_vars*).
   /// \param D OpenMP directive.
----------------
I'd write these as parameter names, i.e. void (*)(kmp_int32 *ThreadID, kip_int32 BoundID, struct context_vars*). The C-style comment right next to the * is really hard to read.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:282
@@ +281,3 @@
+  /// outlined function has type void(*)(kmp_int32 /*ThreadID*/, kmp_int32
+  /// /*PartID*/, struct context_vars*).
+  /// \param D OpenMP directive.
----------------
Same thing. Just write these comments as parameter names.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:737
@@ +736,3 @@
+  llvm::PointerIntPair<llvm::Value *, 1, bool> Final;
+  if (auto *Clause = S.getSingleClause(/*K=*/OMPC_final)) {
+    // If the condition constant folds and can be elided, try to avoid emitting
----------------
You don't need to comment obvious arguments like this (and like a few other places nearby).  The reader understands what the purpose of the argument is, and "K=" doesn't add anything meaningful anyway.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:746
@@ +745,3 @@
+      Final.setPointer(EvaluateExprAsBool(Cond));
+  } else
+    // By default the task is not final.
----------------
Braces.

http://reviews.llvm.org/D7560

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