[cfe-commits] [PATCH] First OpenMP patch

Alexey Bataev a.bataev at hotmail.com
Thu Jan 31 04:53:12 PST 2013























Hello Dmitri, Thank you very much for the review. I agree with almost all your comments and made required fixes.And some answers:1. + class OpenMPVarDeclBitfields {
+ friend class VarDecl;
+ friend class ASTDeclReader;
+
+ unsigned : 32;
+
+ /// \brief is the variable threadprivate
+ unsigned Threadprivate : 1;
+ };

Why did you leave a whole word of padding here?Because there are fields VarDeclBits and ParmVarDeclBits, that take exactly 32 bits.
2. 
+ Token *Toks = new Token[Pragma.size()];

That's a memleak.I don't agree, because this array is passed to PP.EnterTokenStream() with OwnsTokens set to true. In this case this array should be deleted (see TokenLexer::destroy).
3. // Read tokens while ')' or ';' is not found

Why are you doing special processing for a semicolon?Semicolon is the mark that the parser reached the end of the pragma.
4.
+ // A threadprivate directive for static class member variables must appear
+ // in the class definition, in the same scope in which the member
+ // variables are declared

A reference would not hurt here. Also, full stop at the end, and
empty lines to separate paragraphs.

Also, I don't see where is this rule checked. If it is, then could
you add a test:

struct A {
static int a;
};
#pragma omp threadprivate (A::a)

It is checked by the symbol lookup process. It looks only for unqualified symbols. The test is added.
5.
+ // A threadprivate directive for namespace-scope variables must appear
+ // outside any definition or declaration other than the namespace
+ // definition itself

I don't see where this is checked, too. And tests are apparently missing.
The same here.
A new patch is in the attach.Also I uploaded patch to phabricator.
Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team
Intel Corp. 
> From: gribozavr at gmail.com
> Date: Wed, 30 Jan 2013 18:20:58 +0200
> Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
> To: a.bataev at hotmail.com
> CC: cfe-commits at cs.uiuc.edu; dgregor at apple.com
> 
> On Wed, Jan 30, 2013 at 5:16 PM, Alexey Bataev <a.bataev at hotmail.com> wrote:
> > Hello all, Doug,
> > Here is the second patch for OpenMP support in clang. It includes parsing
> > and semantic analysis for #pragma omp threadprivate. It would be good if you
> > take a look at it and send any comments.
> 
> Hello Alexey,
> 
> I am excited to see this patch!
> 
> This is not a full review.  It might be easier to do reviews if you
> uploaded the patch to phabricator.  (But I don't know Doug's
> preference about this.)
> 
>  The list of warnings below should NEVER grow.  It should gradually shrink to 0.
> 
> -CHECK: Warnings without flags (145):
> +CHECK: Warnings without flags (146):
> 
> That comment says what it says.  We should never add a warnings
> without a flag, even temporarily.
> 
> \ No newline at end of file
> 
> Please add a newline.
> 
> Index: test/OpenMP/threadprivate_messages.cpp
> ===================================================================
> --- test/OpenMP/threadprivate_messages.cpp	(revision 0)
> +++ test/OpenMP/threadprivate_messages.cpp	(revision 0)
> 
> threadprivate_diagnostics might be better.
> 
> It is bikeshedding, it might be better to name all entities in the
> test using some consistent scheme.
> 
> +int a; // expected-note {{forward declaration of 'a'}}
> 
> This is not correct.  This is not a forward declaration, this is a
> tentative definition in C, and just a plain definition in C++
> 
> +static __thread int thread_local; // expected-note {{forward
> declaration of 'thread_local'}}
> 
> 'thread_local' is a keyword in C++11.  To future-proof this test,
> choose a different name.
> 
> +#pragma omp threadprivate (staticVar) // expected-error {{undeclared
> variable 'staticVar' used as an argument for '#pragma omp
> threadprivate'}}
> 
> If it is not declared, how do we know it is a variable?
> 
> +#pragma omp threadprivate(argc,ll) // expected-error 2 {{arguments of
> '#pragma omp threadprivate' must have global storage}}
> 
> There is no 'global' storage.  'static storage duration' is needed here.
> 
> +++ include/clang/Basic/OpenMPKinds.h	(revision 0)
> +class Token;
> [...]
> +OpenMPDirectiveKind getOpenMPDirectiveKind(Token Tok);
> 
> This header does not compile when it is the first one included in the TU.
> 
> +def warn_pragma_omp_ignored : Warning <
> +  "unexpected '#pragma omp ...' in program; did you forget one of '-fopenmp' "
> +  "or '-fno-openmp' flags?">;
> 
> I think the consensus was that until our implementation is somewhat
> complete, we should not support or suggest -fopenmp in the driver.
> 
> +  /// ActOnOpenMPThreadprivateDirective - Called on well-formed
> +  /// '\#pragma omp threadprivate'.
> +  DeclGroupPtrTy ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
> 
> When adding new code, please don't duplicate function or class name
> name in the comment.  (There are multiple occurrences of this.)
> 
> +++ include/clang/AST/OpenMP.h	(revision 0)
> 
> I think this should be DeclOpenMP, so it is named like DeclCXX,
> DeclTemplate, etc.
> 
> +//  This file defines the OpenMP directives.
> 
> "This file defines OpenMP AST nodes."  And please also use three
> slashes and "\file".
> 
> +  varlist_iterator varlist_end() {return Vars + NumVars;}
> 
> Spaces after "{" and before "}".  (There are multiple occurrences of this.)
> 
> +  // Implement isa/cast/dyncast/etc.
> 
> You can safely drop this comment.
> 
> +  class OpenMPVarDeclBitfields {
> +    friend class VarDecl;
> +    friend class ASTDeclReader;
> +
> +    unsigned : 32;
> +
> +    /// \brief is the variable threadprivate
> +    unsigned Threadprivate : 1;
> +  };
> 
> Why did you leave a whole word of padding here?
> 
> +    for (OMPThreadPrivateDecl::varlist_iterator P = D->varlist_begin(),
> +                                        E = D->varlist_end();
> 
> Coding style: iterator should be named I, and 'I' and 'E' are usually aligned.
> 
> +OpenMPDirectiveKind clang::getOpenMPDirectiveKind(Token Tok) {
> +  return (llvm::StringSwitch<OpenMPDirectiveKind>(
> +                                  Tok.getIdentifierInfo()->getName())
> 
> Why is this parenthesized?
> 
> +    case (OMPD_unknown):
> 
> Please don't parenthesize 'case' values.
> 
> +/// ActOnOpenMPThreadprivateDirective - Called on well-formed
> +/// '\#pragma omp threadprivate'.
> +Sema::DeclGroupPtrTy
> Sema::ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
> 
> Don't duplicate the comment in .cpp file.
> 
> +Sema::DeclGroupPtrTy
> Sema::ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
> +                                Scope *CurScope,
> +                                const SmallVector<Token, 5> &Identifiers) {
> 
> This should be SmallVectorImpl, or even better, an ArrayRef.
> 
> --- lib/AST/OpenMP.cpp	(revision 0)
> +++ lib/AST/OpenMP.cpp	(revision 0)
> @@ -0,0 +1,53 @@
> +//===--- Decl.cpp - Declaration AST Node Implementation
> -------------------===//
> 
> This comment is not correct.
> 
> +// This file implements the Decl subclasses.
> 
> This one too.
> 
> +void OMPThreadPrivateDecl::setVars(ASTContext &Context, VarDecl **B,
> +                                    const size_t S) {
> 
> (1) Align "ASTContext" and "const".
> (2) Don't constify by-value parameters.
> 
> +  size_t allocationSize = S * sizeof(VarDecl *);
> +  void *buffer = Context.Allocate(allocationSize, sizeof(void *));
> 
> (1) Use llvm::alignOf<>.
> (2) Tail-allocate instead of doing an additional allocation.  See
> CXXTryStmt::Create as an example.
> 
> +    else if (isa<FunctionDecl>(*D) &&
>          cast<FunctionDecl>(*D)->isThisDeclarationADefinition())
> 
> Align 'isa' and 'cast'.
> 
> +//----------------------------------------------------------------------------
> +// OpenMP #pragma omp threadprivate
> +//----------------------------------------------------------------------------
> 
> This kind of ASCII art is discouraged, especially when used to mark a
> single function.
> 
> +  Token *Toks = new Token[Pragma.size()];
> 
> That's a memleak.
> 
> +  void SetWarned() {Warned = true;}
> 
> Function names should start with a lowercase letter.  Also missing
> spaces around "{}".
> 
> +  SourceLocation Loc = ConsumeToken();
> +  switch(getOpenMPDirectiveKind(Tok)) {
> +    case (OMPD_threadprivate): {
> +        SmallVector<Token, 5> Identifiers;
> 
> Weird indentation on "SmallVector" and "case".
> 
> +        if(ParseOpenMPSimpleVarList(OMPD_threadprivate, Identifiers)) {
> Space after 'if'.
> 
> +          return (Actions.ActOnOpenMPThreadprivateDirective(Loc,
> +                                                    getCurScope(),
> +                                                    Identifiers));
> Weird indentation and parentheses.
> 
> +  // Read tokens while ')' or ';' is not found
> 
> Why are you doing special processing for a semicolon?
> 
> +    }
> +    else {
> 
> Should be "} else {".
> 
> +  bool isCorrect = true;
> Variable names should start with an uppercase letter.
> 
> +    // A threadprivate directive for static class member variables must appear
> +    // in the class definition, in the same scope in which the member
> +    // variables are declared
> 
> A reference would not hurt here.  Also, full stop at the end, and
> empty lines to separate paragraphs.
> 
> Also, I don't see where is this rule checked.  If it is, then could
> you add a test:
> 
> struct A {
> static int a;
> };
> #pragma omp threadprivate (A::a)
> 
> +    // A threadprivate directive for namespace-scope variables must appear
> +    // outside any definition or declaration other than the namespace
> +    // definition itself
> 
> I don't see where this is checked, too.  And tests are apparently missing.
> 
> Dmitri
> 
> -- 
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/







 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/fa42660d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.p
Type: text/x-pascal
Size: 57572 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130131/fa42660d/attachment.p>


More information about the cfe-commits mailing list