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

[Alexey Bataev]

  John,
  Thanks for review! See my comments. Updated patch will be prepared soon.

  --
  Best regards,
  Alexey Bataev
  =============
  Software Engineer
  Intel Compiler Team
  Intel Corp.


================
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,
----------------
John McCall wrote:
> This typedef has a very generic-sounding name and a very specific purpose.
Ok, I'll rename it.

================
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
----------------
John McCall wrote:
> 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.
Ok

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

================
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;
----------------
John McCall wrote:
> 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.
I'll try to rework it.

================
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,
----------------
John McCall wrote:
> Please create this global lazily.
Ok

================
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 =
----------------
John McCall wrote:
> Please create this string lazily.
Ok

================
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");
----------------
John McCall wrote:
> Please /*comment*/ arguments like "true".
Ok

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:53
@@ +52,3 @@
+
+llvm::Value *CGOpenMPRuntime::GetOpenMPLocation(CodeGenFunction &CGF,
+                                                SourceLocation Loc,
----------------
John McCall wrote:
> 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?
Ok, I'll rename it.

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

================
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()) {
----------------
John McCall wrote:
> This check is always true; you bailed out earlier if it wasn't.
Agree, I'll remove it

================
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));
----------------
John McCall wrote:
> 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.
Agree

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1
@@ +1,2 @@
+//===--- CGStmtOpenMP.cpp - Emit LLVM Code from Statements ----------------===//
+//
----------------
John McCall wrote:
> There's no reason to make this a separate file.  Just keep all the OpenMP code in one place as much as possible.
I think it would be better to keep a separate file because there should be a lot of code for OpenMP.

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

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

================
Comment at: lib/Sema/SemaOpenMP.cpp:685
@@ +684,3 @@
+  case OMPD_unknown:
+  case NUM_OPENMP_DIRECTIVES:
+    llvm_unreachable("Unknown OpenMP directive");
----------------
John McCall wrote:
> 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.
Ok

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


http://reviews.llvm.org/D2883