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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:1879
+  Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation EndLoc,
+         ArrayRef<OMPClause *> Clauses, Stmt *AssociatedStmt, bool HasCancel);
+
----------------
`HasCancel` param?


================
Comment at: clang/lib/AST/StmtOpenMP.cpp:560
+      C.Allocate(Size + sizeof(OMPClause *) * Clauses.size() + sizeof(Stmt *));
+  OMPParallelMasterDirective *Dir =
+      new (Mem) OMPParallelMasterDirective(StartLoc, EndLoc, Clauses.size());
----------------
`auto *Dir`


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2933
+    OMPPrivateScope PrivateScope(CGF);
+    bool Copyins = CGF.EmitOMPCopyinClause(S);
+    (void)CGF.EmitOMPFirstprivateClause(S, PrivateScope);
----------------
Do we need copyins here at all? We have only master thread active here in this construct.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2950
+  emitCommonOMPParallelDirective(*this, S, OMPD_master, CodeGen,
+                                 emitEmptyBoundParameters);
+}
----------------
Seems to me, missed this code at the end:
```
emitPostUpdateForReductionClause(*this, S,
                                 [](CodeGenFunction &) { return nullptr; });
```


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4015
+    } else if (CurrentRegion == OMPD_master ||
+               CurrentRegion == OMPD_parallel_master) {
       // OpenMP [2.16, Nesting of Regions]
----------------
This is wrong, I believe. you don't have pure `master` region here, it is `parallel master` and it must be allowed to use it in the worksharing and tasking regions.


================
Comment at: clang/test/OpenMP/nesting_of_regions.cpp:2999
+  {
+#pragma omp simd
+    for (int i = 0; i < 10; ++i)
----------------
Are simds allowed in master?


================
Comment at: clang/test/OpenMP/nesting_of_regions.cpp:3024
+  {
+#pragma omp master // OK, though second 'master' is redundant
+    {
----------------
Is this allowed by the standard?


================
Comment at: clang/test/OpenMP/parallel_master_ast_print.cpp:15-21
+int main (int argc, char **argv) {
+  int b = argc, c, d, e, f, g;
+  static int a;
+// CHECK: static int a;
+#pragma omp parallel master
+{
+  a=2;
----------------
Add printing of all supported clauses, take a look at other AST printing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70726