[cfe-commits] [PATCH] First OpenMP patch

[Alexey Bataev]

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.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
>>
>