[cfe-commits] [PATCH] First OpenMP patch

[Dmitri Gribenko]

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>*/