[cfe-commits] [PATCH] First OpenMP patch
Hal Finkel
hfinkel at anl.gov
Wed Feb 6 21:08:12 PST 2013
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.
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?)
+#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 ;)
+#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}}
+#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.
+#pragma omp threadprivate (t) // expected-error {{variable 't' cannot be threadprivate as it is thread-local}}
as -> because
+#pragma omp threadprivate(d2) // expected-error {{'#pragma omp threadprivate' must lexically precede all references to variable 'd2'}}
We can probably remove the word lexically.
+ 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.
-Hal
----- Original Message -----
> From: "Alexey Bataev" <a.bataev at hotmail.com>
> To: gribozavr at gmail.com
> Cc: "cfe commits" <cfe-commits at 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: 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>*/
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list