[PATCH] D70726: [OpenMP50] Add parallel master construct

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Missing check for prohibited nesting of the barrier and worksharing directvies in the master regions. Plus, definitely no tests for this.



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:1857
+  /// true if current directive has inner cancel directive.
+  bool HasCancel;
+
----------------
Why do we need this if cancel in `parallel master` is not supported?


================
Comment at: clang/include/clang/Sema/Sema.h:9564
+  StmtResult ActOnOpenMPParallelMasterDirective(ArrayRef<OMPClause *> Clauses,
+                                                  Stmt *AStmt,
+                                                  SourceLocation StartLoc,
----------------
Not formatted


================
Comment at: clang/lib/AST/StmtOpenMP.cpp:335-349
-OMPSectionsDirective *OMPSectionsDirective::Create(
-    const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc,
-    ArrayRef<OMPClause *> Clauses, Stmt *AssociatedStmt, bool HasCancel) {
-  unsigned Size =
-      llvm::alignTo(sizeof(OMPSectionsDirective), alignof(OMPClause *));
-  void *Mem =
-      C.Allocate(Size + sizeof(OMPClause *) * Clauses.size() + sizeof(Stmt *));
----------------
Why moved to a different place?


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2872-2879
+void CodeGenFunction::EmitMaster(const OMPExecutableDirective &S) {
   auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) {
     Action.Enter(CGF);
     CGF.EmitStmt(S.getInnermostCapturedStmt()->getCapturedStmt());
   };
-  OMPLexicalScope Scope(*this, S, OMPD_unknown);
   CGM.getOpenMPRuntime().emitMasterRegion(*this, CodeGen, S.getBeginLoc());
 }
----------------
Make it static local, not a member function


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10677
       break;
+    case OMPD_parallel_master:
     case OMPD_parallel_master_taskloop:
----------------
`parallel master` is very similar to just `parallel` rather than with `parallel master taskloop` when it comes to the analysis of the clauses.


================
Comment at: clang/test/OpenMP/parallel_master_codegen.cpp:16-25
+// CHECK-LABEL: parallel_master
+void parallel_master() {
+#pragma omp parallel master
+  // CHECK-NOT: __kmpc_global_thread_num
+  // CHECK:     call i32 @__kmpc_master({{.+}})
+  // CHECK:     invoke void {{.*}}foo{{.*}}()
+  // CHECK-NOT: __kmpc_global_thread_num
----------------
Check that the region was outlined. Plus, codegen for some of the clauses are required.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70726/new/

https://reviews.llvm.org/D70726