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

[John McCall]

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1
@@ +1,2 @@
+//===--- CGStmtOpenMP.cpp - Emit LLVM Code from Statements ----------------===//
+//
----------------
There's no reason to make this a separate file.  Just keep all the OpenMP code in one place as much as possible.

================
Comment at: include/clang/Sema/Sema.h:2984
@@ -2983,1 +2983,3 @@
                                 CapturedRegionKind Kind, unsigned NumParams);
+  typedef std::pair<StringRef, QualType> ParamNameType;
+  void ActOnCapturedRegionStart(SourceLocation Loc, Scope *CurScope,
----------------
This typedef has a very generic-sounding name and a very specific purpose.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:78
@@ +77,3 @@
+  /// http://llvm.org/svn/llvm-project/openmp/trunk/runtime/src/kmp.h
+  enum Ident_TFields {
+    /// \brief might be used in Fortran
----------------
The _T suffix makes this pretty confusing to read — it's ident_t in the original, presumably, and capitalizing it does it no favors.

I would rename:
  Ident_TTy -> IdentTy
  Ident_TFields -> IdentFieldIndex
  Ident_T_* -> IdentField_*

Also, put this enum and its comment next to the declaration of IdentTy, and include a C-style struct definition in the comment.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:97
@@ +96,3 @@
+  llvm::StructType *Ident_TTy;
+  llvm::FunctionType *Kmpc_MicroTy;
+  llvm::GlobalVariable *DefaultOpenMPLocation;
----------------
Please add a comment to the end of the line describing this function type in C terms, if reasonable.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:100
@@ +99,3 @@
+  /// \brief Map of local debug location and functions.
+  typedef llvm::DenseMap<llvm::Function *, llvm::AllocaInst *> OpenMPLocMapTy;
+  OpenMPLocMapTy OpenMPLocMap;
----------------
Does this need to be as specific as an AllocaInst?

Also, these maps seem to contain information that's really specific to a CodeGenFunction; you should get notified when a CGF is torn down so that you can remove entries as appropriate.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:47
@@ +46,3 @@
+  llvm::Constant *Init = llvm::ConstantStruct::get(Ident_TTy, Values);
+  DefaultOpenMPLocation = new llvm::GlobalVariable(
+      CGM.getModule(), Ident_TTy, true, llvm::GlobalValue::PrivateLinkage, Init,
----------------
Please create this global lazily.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:48
@@ +47,3 @@
+  DefaultOpenMPLocation = new llvm::GlobalVariable(
+      CGM.getModule(), Ident_TTy, true, llvm::GlobalValue::PrivateLinkage, Init,
+      ".kmpc_default_loc.addr");
----------------
Please /*comment*/ arguments like "true".

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:62
@@ +61,3 @@
+
+  SmallString<32> Buffer;
+  llvm::raw_svector_ostream OS(Buffer);
----------------
Remove this entire thing and use an llvm::Twine inline in the argument to CreateTempAlloca.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:91
@@ +90,3 @@
+      CGF.Builder.CreateConstInBoundsGEP2_32(AI, 0, Ident_T_PSource);
+  if (CGM.getCodeGenOpts().getDebugInfo() != CodeGenOptions::NoDebugInfo) {
+    if (Loc.isValid()) {
----------------
This check is always true; you bailed out earlier if it wasn't.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:53
@@ +52,3 @@
+
+llvm::Value *CGOpenMPRuntime::GetOpenMPLocation(CodeGenFunction &CGF,
+                                                SourceLocation Loc,
----------------
I would not expect this method to cause code to be emitted at the current insertion point.  Maybe call it emitUpdateLocation or something like that?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:31
@@ +30,3 @@
+  // http://llvm.org/svn/llvm-project/openmp/trunk/runtime/src/kmp_str.c
+  DefaultOpenMPPSource = CGM.GetAddrOfConstantCString(";unknown;unknown;0;0;;");
+  DefaultOpenMPPSource =
----------------
Please create this string lazily.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:125
@@ +124,3 @@
+    // Generate "int32 .kmpc_global_thread_num.addr;"
+    AI = CGF.CreateTempAlloca(CGF.Int32Ty, ".kmpc_global_thread_num.addr");
+    AI->setAlignment(CGM.getDataLayout().getPrefTypeAlignment(CGF.Int32Ty));
----------------
I'm not sure why you're using an alloca if you're going to unconditionally do the call in a place that dominates the entire function.  Just do the call and use the result wherever you need it.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:32
@@ +31,3 @@
+  llvm::Value *OutlinedFn;
+  {
+    CGCapturedStmtInfo CGInfo(*CS, CS->getCapturedRegionKind());
----------------
You should declare CGF inside this block, too.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:42
@@ +41,3 @@
+    CGM.getOpenMPRuntime().GetOpenMPLocation(*this, S.getLocStart()),
+    Builder.getInt32(1),
+    Builder.CreateBitCast(OutlinedFn,
----------------
Please comment this magic number.

================
Comment at: lib/Sema/SemaOpenMP.cpp:685
@@ +684,3 @@
+  case OMPD_unknown:
+  case NUM_OPENMP_DIRECTIVES:
+    llvm_unreachable("Unknown OpenMP directive");
----------------
Please define your enums so that things like this case aren't necessary.  If you really need a NUM_OPENMP_DIRECTIVES constant, you can define it right after the enum.

================
Comment at: lib/Sema/SemaOpenMP.cpp:745
@@ +744,3 @@
+  // Mark captured decl as nothrow
+  CS->getCapturedDecl()->setNothrow();
+
----------------
This comment is useless.  Please instead cite *why* you can mark the captured decl as no throw.


http://reviews.llvm.org/D2883