Hello Hal,
Thank you for review.
My answers are below.
Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team
Intel Corp.
07.02.2013 9:08, Hal Finkel пишет:
Alexey,
Thanks for working on this! A few comments:
+#pragma omp threadprivate (m) // expected-error {{use of undeclared identifier
'm'; did you mean 'ns::m'?}}
This is not good... it seems to be suggesting a qualified name, but qualified
names can't be used in the directives.
Agree, I'll fix it.
Also, is there any real reason not to accept qualified names? OpenMP 3 may not
explicitly permit it, but it seems like a reasonable extension, and should be
allowed in OpenMP 4.0. OpenMP 4.0 RC 1, appendix C, says:
# pragma omp threadprivate ( variable-list ) new-line
where variable-list (in C++) is explicitly a list of id-expression, and
id-expression in C++ includes qualified names (right?)
No, I think it is not quite correct. Here is an excerpt from OpenMP 4.0
RC 1 (2.11.2, Restrictions):
A threadprivate directive for namespace-scope variables must appear
outside any definition or declaration other than the namespace
definition itself, and must lexically precede all references to any of
the variables in its list.
+#pragma omp threadprivate (n::m) // expected-error {{expected ',' or ')' in
'#pragma omp threadprivate'}}
If we're doing to disallow qualified names, we should produce a specific error
message that qualified names are not allowed. Clang is supposed to be the
compiler with the best error messages ;)
Agree.
+#pragma omp threadprivate (h, i) // expected-error {{the address of variable
'h' must not be an address constant}}
Also, I find this error message confusing. Can we just say, {{threadprivate variables
must not be <type> constants}}
I tried to make error messages similar to the definitions in OpenMP
specification document. But if you think this message is not quite good,
I can modify it.
+#pragma omp parallel private() // expected-error {{unexpected OpenMP directive
'parallel'}}
+#pragma omp omp_pragma // expected-error {{unknown OpenMP directive
'omp_pragma'}}
Why are these in the threadprivate_messages.cpp test? I think they should be in
a basic OpenMP test.
Agree, I'll modify this test.
+#pragma omp threadprivate (t) // expected-error {{variable 't' cannot be
threadprivate as it is thread-local}}
as -> because
Ok.
+#pragma omp threadprivate(d2) // expected-error {{'#pragma omp threadprivate'
must lexically precede all references to variable 'd2'}}
We can probably remove the word lexically.
Again, I tried to keep wordings of the OpenMP specification.
+ if (Lookup.isAmbiguous())
+ continue;
Was there a test for this? Regarding qualified names, I'm worried this might
suggest qualified names that the user can't use.
No, there is no test. I'll try to add test and try to solve the problem
with the qualified names.
-Hal
----- Original Message -----
From: "Alexey Bataev" <a.bat...@hotmail.com>
To: griboz...@gmail.com
Cc: "cfe commits" <cfe-commits@cs.uiuc.edu>
Sent: Thursday, January 31, 2013 6:53:12 AM
Subject: RE: [cfe-commits] [PATCH] First OpenMP patch
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: griboz...@gmail.com
Date: Wed, 30 Jan 2013 18:20:58 +0200
Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
To: a.bat...@hotmail.com
CC: cfe-commits@cs.uiuc.edu; dgre...@apple.com
On Wed, Jan 30, 2013 at 5:16 PM, Alexey Bataev
<a.bat...@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 <griboz...@gmail.com>*/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
