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

[Alexey Bataev]

John, thanks for the review!


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:118
@@ +117,3 @@
+  auto KmpRoutineEntryPointerQTy = C.getPointerType(KmpRoutineEntryTy);
+  KmpRoutineEntryPtrTy = CGM.getTypes().ConvertType(KmpRoutineEntryPointerQTy);
+  // Build struct kmp_task_t.
----------------
rjmccall wrote:
> Build this type lazily, please.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:119
@@ +118,3 @@
+  KmpRoutineEntryPtrTy = CGM.getTypes().ConvertType(KmpRoutineEntryPointerQTy);
+  // Build struct kmp_task_t.
+  auto *RD = C.buildImplicitRecord("kmp_task_t");
----------------
rjmccall wrote:
> A better way of commenting the next few lines would be to just write the struct definition out and then put all the addFieldToRecordDecl calls together.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:133
@@ -94,1 +132,3 @@
+  auto KmpTaskQTy = C.getRecordType(KmpTaskTRD);
+  KmpTaskTTy = CGM.getTypes().ConvertType(KmpTaskQTy);
 }
----------------
rjmccall wrote:
> Build this type lazily, please.
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:252
@@ -210,4 +251,3 @@
     auto RVal = CGF.EmitLoadOfLValue(LVal, Loc);
-    LVal = CGF.MakeNaturalAlignAddrLValue(RVal.getScalarVal(),
-                                          ThreadIDVar->getType());
-    ThreadID = CGF.EmitLoadOfLValue(LVal, Loc).getScalarVal();
+    if (ThreadIDVar->getType()->isPointerType()) {
+      // Thread id is passed as a pointer
----------------
rjmccall wrote:
> It looks like the invariants about getThreadIDVariable have changed; it used to always be an int32*, and now it can be either that or an int32.  EmitThreadIDAddress appears to still assume that it's an ident*.  You should fix that and then add a test that the OpenMP constructs using that still work correctly within a task.
> 
> You should also document this on CGOpenMPRegionInfo::ThreadIDVar.
I'll fix it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:257
@@ +256,3 @@
+      ThreadID = CGF.EmitLoadOfLValue(LVal, Loc).getScalarVal();
+    } else
+      // Thread id is passed as a value (in tasks).
----------------
rjmccall wrote:
> Missing braces.
My bad, will be fixed

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1012
@@ +1011,3 @@
+static RecordDecl *createNewKmpTaskTRecordDecl(ASTContext &C,
+                                               const RecordDecl *RD) {
+  auto *NewRD = C.buildImplicitRecord("kmp_task_t");
----------------
rjmccall wrote:
> Comment, please.  Please include the fact that this leaves the RecordDecl incomplete.
> 
> Also, RD has to be KmpTaskTRD, so remove that argument and make this take the CGOpenMPRuntime.
Ok, I'll improve it

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1093
@@ +1092,3 @@
+  // kmp_int32 flags, size_t sizeof_kmp_task_t, size_t sizeof_shareds,
+  // kmp_routine_entry_t *task_entry);
+  // Task flags. Format is taken from
----------------
rjmccall wrote:
> Please add a newline after this comment to make it clear that it's describing the next sequence of steps as a whole.
Ok, will be done.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:17
@@ -16,2 +16,3 @@
 
+#include "clang/AST/Type.h"
 #include "clang/Basic/OpenMPKinds.h"
----------------
rjmccall wrote:
> I don't think you need this; you can just forward-declare QualType.  If you do need this, you can remove the forward-declaration of RecordDecl, I am positive that Type.h does that already.
I'll fix it.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:288
@@ -261,1 +287,3 @@
+  /// /*ThreadID*/, kmp_int32 /*PartID*/, struct context_vars*) (if PartIdVar !=
+  /// nullptr).
   /// \param D OpenMP directive.
----------------
rjmccall wrote:
> I was concerned about awkwardly making a single function do completely different things based on weird defaulted parameter, but it turns out that those types are exactly the same.
> 
> Would it be more appropriate to say that this creates an outlined function, and that if PartID is given, it's bound to the appropriate parameter value?
> 
> Although it also looks like PartIDVar is completely ignored except for an assertion.  Is that expected to change?
I'll try to split this function in two - one for tasks and another one for parallel regions. I think this is better, rather than keep too complex single function.

================
Comment at: lib/Sema/SemaOpenMP.cpp:1124
@@ -1124,2 +1123,3 @@
   case OMPD_task: {
+    QualType KmpInt32Ty = Context.getIntTypeForBitwidth(32, 1);
     Sema::CapturedParamNameType Params[] = {
----------------
rjmccall wrote:
> This seems be hard-coding KMP runtime assumptions into Sema and the AST.  Is that unavoidable?
Unfortunately, no. I'm trying to avoid runtime assumptions in common code, but sometimes (like function parameters for outlined functions) I'm just forced to make some hard-coding of runtime specific features.

http://reviews.llvm.org/D7560

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