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

[Alexey Bataev]

John, thanks for the review!


================
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);
 
----------------
rjmccall wrote:
> 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.
Ok, will do

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

================
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) {
----------------
rjmccall wrote:
> Closing parenthesis.  Also, I'd make a getter for this instead of inlining it here.
Ok, agree, that getter would look much better.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1230
@@ +1229,3 @@
+  auto *TaskEntry = llvm::Function::Create(
+      TaskEntryTy, /*Linkage=*/llvm::GlobalValue::InternalLinkage,
+      ".omp_task_entry.", &CGM.getModule());
----------------
rjmccall wrote:
> 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.
Ok, removed

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1266
@@ +1265,3 @@
+                                              KmpInt32Ty));
+  TaskEntryCGF.FinishFunction();
+
----------------
rjmccall wrote:
> 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.
Agree, I will create a separate function for proxy 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.
----------------
rjmccall wrote:
> 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.
I don't think I need to call a copy constructor here. Actually structure with a list of shared variables is always POD type (it stores only pointers and integer values), so we can simply copy it

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1318
@@ +1317,3 @@
+                             getThreadID(CGF, Loc), NewTask};
+  CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__kmpc_omp_task), TaskArgs);
+}
----------------
rjmccall wrote:
> Nothing cares about the result of this call?
The check is required for untied tasks only, but they are not supported yet. I'll add TODO here.

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

================
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.
----------------
rjmccall wrote:
> Same thing. Just write these comments as parameter names.
Fixed

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

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

http://reviews.llvm.org/D7560

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