[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