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

[Hal Finkel]

----- Original Message -----
> From: "Alexey Bataev" <a.bataev at hotmail.com>
> To: dgregor at apple.com, gribozavr at gmail.com, hfinkel at anl.gov, fraggamuffin at gmail.com, cbergstrom at pathscale.com,
> richard at metafoo.co.uk, "a bataev" <a.bataev at hotmail.com>
> Cc: cfe-commits at cs.uiuc.edu
> Sent: Friday, March 7, 2014 5:56:04 AM
> Subject: Re: [PATCH] [OPENMP] Initial codegen for '#pragma omp parallel'
> 
> 
> 
> ================
> Comment at: lib/CodeGen/CGOpenMPRuntime.h:45
> @@ +44,3 @@
> +public:
> +  enum OpenMPLocationFlags {
> +    /// \brief Use tramponline for internal microtask.
> ----------------
> hfinkel at anl.gov wrote:
> > This should have some overall comment explaining what this is the
> > location of.
> Ok, I'll add comment
> 
> ================
> Comment at: lib/CodeGen/CGOpenMPRuntime.h:48
> @@ +47,3 @@
> +    OMP_IDENT_IMD = 0x01,
> +    /// \brief Use c-stile ident structure.
> +    OMP_IDENT_KMPC = 0x02,
> ----------------
> hfinkel at anl.gov wrote:
> > c-style?
> Yes, all these comments are taken from
> http://llvm.org/svn/llvm-project/openmp/trunk/runtime/src/kmp.h

I was wondering whether 'c-stile' was a typo, and should have been c-style?

 -Hal

> 
> ================
> Comment at: lib/CodeGen/CGOpenMPRuntime.h:72
> @@ +71,3 @@
> +private:
> +  enum Ident_TFields {
> +    Ident_T_Reserved_1,
> ----------------
> hfinkel at anl.gov wrote:
> > This needs to be documented (including the fields too).
> Ok, will be commented.
> 
> ================
> Comment at: lib/CodeGen/CGOpenMPRuntime.h:80
> @@ +79,3 @@
> +  CodeGenModule &CGM;
> +  llvm::Constant *DefaultOpenMPPSource;
> +  llvm::StructType *Ident_TTy;
> ----------------
> hfinkel at anl.gov wrote:
> > What's this?
> Default position used for initialization of all ident_t objects. I'll
> described it.
> 
> ================
> Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:28
> @@ +27,3 @@
> +CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM) : CGM(CGM) {
> +  DefaultOpenMPPSource =
> CGM.GetAddrOfConstantCString(";unknown;unknown;0;0;;");
> +  DefaultOpenMPPSource =
> ----------------
> hfinkel at anl.gov wrote:
> > Please document the meaning of this string.
> Ok.
> 
> ================
> Comment at: lib/Sema/SemaOpenMP.cpp:675
> @@ +674,3 @@
> +        { std::make_pair("global_tid", KmpInt32PtrTy),
> +          std::make_pair("bound_tid", KmpInt32PtrTy),
> +          std::make_pair(StringRef(), QualType()) // __context with
> shared vars
> ----------------
> hfinkel at anl.gov wrote:
> > Could these names conflict with user-provided names? (Should they
> > be in the implementation namespace?)
> Yes, that's possible. I'll rename them.
> 
> 
> http://llvm-reviews.chandlerc.com/D2883
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory