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

[John McCall]

Sorry for the delay.

In general, your code is very difficult to read; it doesn't use any vertical whitespace within functions at all, so I can't easily understand how things are supposed to be grouped.

The basic implementation seems fine, though I have a number of requests:


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

================
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");
----------------
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.

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

================
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
----------------
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.

================
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).
----------------
Missing braces.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1009
@@ +1008,3 @@
+};
+} // namespace
+
----------------
Somewhere around here would be a good place to put the code that lazily builds KmpTaskTRD.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:1012
@@ +1011,3 @@
+static RecordDecl *createNewKmpTaskTRecordDecl(ASTContext &C,
+                                               const RecordDecl *RD) {
+  auto *NewRD = C.buildImplicitRecord("kmp_task_t");
----------------
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.

================
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
----------------
Please add a newline after this comment to make it clear that it's describing the next sequence of steps as a whole.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:17
@@ -16,2 +16,3 @@
 
+#include "clang/AST/Type.h"
 #include "clang/Basic/OpenMPKinds.h"
----------------
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.

================
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.
----------------
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?

================
Comment at: lib/Sema/SemaOpenMP.cpp:1124
@@ -1124,2 +1123,3 @@
   case OMPD_task: {
+    QualType KmpInt32Ty = Context.getIntTypeForBitwidth(32, 1);
     Sema::CapturedParamNameType Params[] = {
----------------
This seems be hard-coding KMP runtime assumptions into Sema and the AST.  Is that unavoidable?

http://reviews.llvm.org/D7560

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