[PATCH] D10765: [OPENMP] Parsing and sema support for #pragma omp target data" directive.

[Alexey Bataev]

Seems to me, the updated patch is not correct. It is missing a lot of required thing. Besides, new tests were removed from updated patch


================
Comment at: include/clang-c/Index.h:2230
@@ -2229,3 +2229,3 @@
    */
-  CXCursor_OMPTaskgroupDirective          = 254,
+  CXCursor_OMPTaskgroupDirective         = 254,
 
----------------
Add a cursor for TargetData directive

================
Comment at: include/clang/AST/DataRecursiveASTVisitor.h:2372
@@ -2371,2 +2371,3 @@
                   { TRY_TO(TraverseOMPExecutableDirective(S)); })
 
+DEF_TRAVERSE_STMT(OMPTargetDataDirective,
----------------
Add recursive visitor for the new directive

================
Comment at: include/clang/AST/RecursiveASTVisitor.h:2405
@@ -2404,2 +2404,3 @@
                   { TRY_TO(TraverseOMPExecutableDirective(S)); })
 
+DEF_TRAVERSE_STMT(OMPTargetDataDirective,
----------------
Add recursive visitor for the new directive

================
Comment at: include/clang/Basic/OpenMPKinds.def:95
@@ -91,3 +94,3 @@
 OPENMP_DIRECTIVE(flush)
 OPENMP_DIRECTIVE(ordered)
 OPENMP_DIRECTIVE(atomic)
----------------
Add definition of the target data directive

================
Comment at: include/clang/Basic/OpenMPKinds.def:272
@@ -267,3 +271,3 @@
 OPENMP_ATOMIC_CLAUSE(seq_cst)
 
 // Clauses allowed for OpenMP directive 'target'.
----------------
There is support for if clause, but macro OPENMP_TARGET_DATA_CLAUSE() is not defined and if clause is not associated with target data directive

================
Comment at: include/clang/Basic/StmtNodes.td:206
@@ -205,2 +205,3 @@
 def OMPAtomicDirective : DStmt<OMPExecutableDirective>;
 def OMPTargetDirective : DStmt<OMPExecutableDirective>;
+def OMPTargetDataDirective : DStmt<OMPExecutableDirective>;
----------------
Add a node for target data directive

================
Comment at: include/clang/Serialization/ASTBitCodes.h:1397
@@ -1396,2 +1396,3 @@
       STMT_OMP_ATOMIC_DIRECTIVE,
       STMT_OMP_TARGET_DIRECTIVE,
+      STMT_OMP_TARGET_DATA_DIRECTIVE,
----------------
Add a serialization code for target data directive

================
Comment at: lib/CodeGen/CGExpr.cpp:1961
@@ -1960,5 +1960,3 @@
                                          CapturedStmtInfo->getContextValue());
-      }
-      assert(isa<BlockDecl>(CurCodeDecl));
-      return MakeAddrLValue(GetAddrOfBlockDecl(VD, VD->hasAttr<BlocksAttr>()),
-                            T, Alignment);
+      } else if (isa<BlockDecl>(CurCodeDecl))
+        return MakeAddrLValue(GetAddrOfBlockDecl(VD, VD->hasAttr<BlocksAttr>()),
----------------
Bad solution, restore original code. Instead you should capture variables used in the target data directive

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2101-2118
@@ -2097,1 +2100,20 @@
+
+void CodeGenFunction::EmitOMPIfClause(const Expr *IfCond) {
+  auto ThenBlock = createBasicBlock("omp.if.then");
+  auto ElseBlock = createBasicBlock("omp.if.else");
+  auto ContBlock = createBasicBlock("omp.if.end");
+
+  EmitBranchOnBoolExpr(IfCond, ThenBlock, ElseBlock, 0);
+
+  // generate the else block
+  EmitBlock(ElseBlock);
+  EmitBranch(ContBlock);
+
+  // generate the then block
+  EmitBlock(ThenBlock);
+  EmitBranch(ContBlock);
+
+  EmitBlock(ContBlock);
+}
+
 // Generate the instructions for '#pragma omp target data' directive.
----------------
In general, this a bad solution. Look at static void emitOMPIfClause() in CGOpenMPRuntime.cpp for better codegen. Your solution won't work with cleanups and exceptions.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2125-2127
@@ +2124,5 @@
+  const Expr *IfCond = nullptr;
+  if (auto C = S.getSingleClause(OMPC_if)) {
+    IfCond = cast<OMPIfClause>(C)->getCondition();
+  }
+
----------------
Remove braces

================
Comment at: lib/CodeGen/CodeGenFunction.h:2203
@@ -2202,2 +2202,3 @@
   void EmitOMPTeamsDirective(const OMPTeamsDirective &S);
+  void EmitOMPIfClause(const Expr *Cond);
 
----------------
This one may be just a static function in CGStmtOpenMP.cpp

================
Comment at: lib/Parse/ParseOpenMP.cpp:38
@@ -37,1 +37,3 @@
+    { OMPD_parallel, OMPD_sections, OMPD_parallel_sections },
+    { OMPD_target, OMPD_unknown, OMPD_target_data }
   };
----------------
Move it to the beginning of the array.

================
Comment at: lib/Parse/ParseOpenMP.cpp:54
@@ -63,3 +53,3 @@
       if (SDKind == F[i][1]) {
-        P.ConsumeToken();
-        DKind = F[i][2];
+        auto IsTargetData = (Tok.isNot(tok::annot_pragma_openmp_end) &&
+                             DKind == OMPD_target &&
----------------
Update your patch to the latest revision and sync it with it


http://reviews.llvm.org/D10765