[PATCH] [OPENMP] Initial codegen for '#pragma omp parallel'

[Alexey Bataev]

  Richard, thanks for the review!!! I do appreciate it.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:49-64
@@ +48,18 @@
+  enum OpenMPLocationFlags {
+    /// \brief Use tramponline for internal microtask.
+    OMP_IDENT_IMD = 0x01,
+    /// \brief Use c-style ident structure.
+    OMP_IDENT_KMPC = 0x02,
+    /// \brief Atomic reduction option for kmpc_reduce.
+    OMP_ATOMIC_REDUCE = 0x10,
+    /// \brief Explicit 'barrier' directive.
+    OMP_IDENT_BARRIER_EXPL = 0x20,
+    /// \brief Implicit barrier in code.
+    OMP_IDENT_BARRIER_IMPL = 0x40,
+    /// \brief Implicit barrier in 'for' directive.
+    OMP_IDENT_BARRIER_IMPL_FOR = 0x40,
+    /// \brief Implicit barrier in 'sections' directive.
+    OMP_IDENT_BARRIER_IMPL_SECTIONS = 0xC0,
+    /// \brief Implicit barrier in 'single' directive.
+    OMP_IDENT_BARRIER_IMPL_SINGLE = 0x140
+  };
----------------
Richard Smith wrote:
> Richard Smith wrote:
> > Our coding convention doesn't use ALL_CAPS here. These should probably be OLF_IdentImd, OLF_IdentKmpc, ... to follow that convention (though I'm not really sure what these names mean, and they don't seem to directly correspond to the comments. What does IMD mean, or KMPC, for instance?)
> > 
> > Alternatively, you could explicitly state that these are named to match the names from kmp.h, rather than just saying the descriptions are from there.
> Typo of 'trampoline'.
Richard, ok, I'll add required comment

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:49-50
@@ +48,4 @@
+  enum OpenMPLocationFlags {
+    /// \brief Use tramponline for internal microtask.
+    OMP_IDENT_IMD = 0x01,
+    /// \brief Use c-style ident structure.
----------------
Alexey Bataev wrote:
> Richard Smith wrote:
> > Richard Smith wrote:
> > > Our coding convention doesn't use ALL_CAPS here. These should probably be OLF_IdentImd, OLF_IdentKmpc, ... to follow that convention (though I'm not really sure what these names mean, and they don't seem to directly correspond to the comments. What does IMD mean, or KMPC, for instance?)
> > > 
> > > Alternatively, you could explicitly state that these are named to match the names from kmp.h, rather than just saying the descriptions are from there.
> > Typo of 'trampoline'.
> Richard, ok, I'll add required comment
Will be fixed

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:53-68
@@ +52,18 @@
+
+namespace {
+/// \brief RAII object that save current insert position and then restores it.
+class BuilderInsertPositionRAII {
+  CGBuilderTy &Builder;
+  CGBuilderTy::InsertPoint SavedIP;
+
+public:
+  BuilderInsertPositionRAII(CGBuilderTy &Builder,
+                            llvm::Instruction *NewInsertPoint)
+      : Builder(Builder), SavedIP(Builder.saveIP()) {
+    assert(SavedIP.isSet() && "No insertion point is set!");
+    Builder.SetInsertPoint(NewInsertPoint);
+  }
+  ~BuilderInsertPositionRAII() { Builder.restoreIP(SavedIP); }
+};
+}
+
----------------
Richard Smith wrote:
> Looks like this can be replaced by `IRBuilder::InsertPointGuard`?
Yes, thanks, missed this one!

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:174
@@ +173,3 @@
+    RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_fork_call");
+  } break;
+  case OMPRTL__kmpc_global_thread_num: {
----------------
Richard Smith wrote:
> We put the `break;` inside the braces.
Thanks, will be fixed.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:31-35
@@ +30,7 @@
+  CodeGenFunction CGF(CGM, true);
+  CGF.CapturedStmtInfo =
+      new CGCapturedStmtInfo(*CS, CS->getCapturedRegionKind());
+  llvm::Value *OutlinedFn = CGF.GenerateCapturedStmtFunction(
+      CS->getCapturedDecl(), CS->getCapturedRecordDecl(), CS->getLocStart());
+  delete CGF.CapturedStmtInfo;
+
----------------
Richard Smith wrote:
> Why are you creating the `CGCapturedStmtInfo` on the heap? Seems weird, since it looks like you delete it two lines afterwards.
I just followed the standard pattern for CapturedStmt (see CGStmt.cpp, CodeGenFunction::EmitCapturedStmt). But I'll rework it.

================
Comment at: lib/Sema/SemaOpenMP.cpp:673-680
@@ +672,10 @@
+    QualType KmpInt32PtrTy = Context.getPointerType(KmpInt32Ty);
+    Sema::ParamNameType Params[3] =
+        { std::make_pair("__global_tid", KmpInt32PtrTy),
+          std::make_pair("__bound_tid", KmpInt32PtrTy),
+          std::make_pair(StringRef(), QualType()) // __context with shared vars
+        };
+    ActOnCapturedRegionStart(Loc, CurScope, CR_OpenMP, Params);
+    }
+    break;
+  case OMPD_threadprivate:
----------------
Richard Smith wrote:
> Formatting here doesn't match our usual conventions. For the braced initialization, put the `{` on the previous line, dedent the `};`. Dedent the `}` for the `case`, and move the `break;` before it.
Ok. I'll reformat it.

================
Comment at: lib/Sema/SemaOpenMP.cpp:685-686
@@ +684,4 @@
+  case OMPD_unknown:
+  case NUM_OPENMP_DIRECTIVES:
+    llvm_unreachable("Unknown OpenMP directive");
+  }
----------------
Richard Smith wrote:
> Yuck. `NUM_OPENMP_DIRECTIVES` shouldn't be a member of the enum, to remove the need to do this.
This one is needed for checking that the directive is correctly specified.

================
Comment at: lib/Sema/TreeTransform.h:9784
@@ +9783,3 @@
+  SmallVector<Sema::ParamNameType, 4> Params;
+  for (unsigned i = 0; i < NumParams; ++i) {
+    if (i != ContextParamPos) {
----------------
Richard Smith wrote:
> Capital `I` please.
Ok

================
Comment at: lib/Serialization/ASTReaderDecl.cpp:1080-1088
@@ -1079,5 +1079,11 @@
   VisitDecl(CD);
+  unsigned ContextParamPos = Record[Idx++];
+  CD->setNothrow(Record[Idx++] != 0);
   // Body is set by VisitCapturedStmt.
-  for (unsigned i = 0; i < CD->NumParams; ++i)
-    CD->setParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+  for (unsigned i = 0; i < CD->NumParams; ++i) {
+    if (i != ContextParamPos)
+      CD->setParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+    else
+      CD->setContextParam(i, ReadDeclAs<ImplicitParamDecl>(Record, Idx));
+  }
 }
----------------
Richard Smith wrote:
> Do you have test coverage for this?
Yes, parallel_codegen.cpp and parallel_ast_print.cpp.


http://reviews.llvm.org/D2883