[PATCH] OpenMP threadprivate directive
Doug Gregor
dgregor at apple.com
Sun Feb 17 23:38:00 PST 2013
Looks like a good start; see comments for a number of minor things and a few more interesting design questions (esp. about the thread-private bit).
================
Comment at: include/clang/Basic/DiagnosticParseKinds.td:773
@@ +772,3 @@
+def warn_pragma_omp_ignored : Warning <
+ "unexpected '#pragma omp ...' in program">, InGroup<SourceUsesOpenMP>;
+def err_omp_unknown_directive : Error <
----------------
This warning (and all warnings in -Wsource-uses-openmp) should be DefaultIgnore. When OpenMP is fully-supported, we can consider turning these warnings on by default and suggesting -fopenmp.
================
Comment at: include/clang/Sema/Sema.h:6537-6539
@@ +6536,5 @@
+ /// \brief Called on well-formed '\#pragma omp threadprivate'.
+ DeclGroupPtrTy ActOnOpenMPThreadprivateDirective(SourceLocation Loc,
+ Scope *CurScope,
+ const ArrayRef<DeclarationNameInfo> &Identifiers);
+
----------------
Dmitri Gribenko wrote:
> Don't leave `Loc` on the previous line. Also, drop `const` from `ArrayRef`, since all its methods are already `const`.
>
> Same for the function definition.
>
ArrayRefs should be passed by value, not be reference.
================
Comment at: lib/AST/DeclOpenMP.cpp:50
@@ +49,3 @@
+ SourceLocation L, VarDecl **VL,
+ size_t N)
+ : Decl(DK, DC, L), NumVars(N) {
----------------
Please use ArrayRef<VarDecl*> rather than VarDecl**/size_t. It's not everywhere in the Clang code base, but it's the preferred style now.
================
Comment at: lib/AST/DeclOpenMP.cpp:58
@@ +57,3 @@
+
+void OMPThreadPrivateDecl::setVars(VarDecl **VL) {
+ if (VL) {
----------------
ArrayRef here, please.
================
Comment at: lib/Frontend/InitPreprocessor.cpp:645
@@ -644,3 +644,3 @@
// OpenMP definition
- if (LangOpts.OpenMP) {
+ if (LangOpts.getOpenMP()) {
// OpenMP 2.2:
----------------
This still triggers with -fno-openmp, doesn't it? Eliminating the three-state OpenMP mode would solve the problem.
================
Comment at: lib/Parse/ParseOpenMP.cpp:95
@@ +94,3 @@
+ SkipUntil(tok::comma, tok::r_paren, true, true);
+ Diag(PrevTok.getLocation(), diag::err_expected_unqualified_id) <<
+ getLangOpts().CPlusPlus <<
----------------
LLVM style prefers that the <<'s go at the beginning of lines, not at the end. This occurs a number of times throughout the patch.
================
Comment at: lib/Parse/ParsePragma.cpp:751
@@ +750,3 @@
+ Tok.startToken();
+ Tok.setKind(tok::semi);
+ Tok.setLocation(EodLoc);
----------------
tok::semi is a poor choice of delimiter here, because it could show up in an (ill-formed) pragma. Why not use tok::eod, which is reserved for this purpose?
================
Comment at: lib/Parse/ParseStmt.cpp:292
@@ +291,3 @@
+
+ case tok::annot_pragma_openmp:
+ SourceLocation DeclStart = Tok.getLocation();
----------------
Why isn't this handled in the default case, if the pragma will always end up being a declaration? Or do you expect this to get more interesting in the future?
================
Comment at: lib/Sema/SemaOpenMP.cpp:49
@@ +48,3 @@
+ FilterLookupForScope(Lookup, getCurLexicalContext(), CurScope,
+ false, false);
+ LookupParsedName(Lookup, CurScope, NULL, true);
----------------
It doesn't make sense to filter the lookup results before you've done the actual lookup. Besides, you don't want to filter things that are out-of-scope, you want to detect them later on and complain about that mistake directly.
================
Comment at: lib/Sema/SemaOpenMP.cpp:58
@@ +57,3 @@
+ VarDeclFilterCCC Validator(*this);
+ DiagnoseEmptyLookup(CurScope, ScopeSpec, Lookup, Validator);
+ continue;
----------------
DiagnoseEmptyLookup recovers using typo correction when possible. You need to detect that case and act as if the user had written the right thing, not "continue" as you would if no correction occurred.
================
Comment at: lib/Sema/SemaOpenMP.cpp:73
@@ +72,3 @@
+ FixItHint::CreateReplacement(I->getLoc(), CorrectedStr);
+ continue;
+ }
----------------
This is an odd bit of recovery. If name lookup finds something that isn't a VarDecl, it's attempting to typo correct? Shouldn't it simply complain if the thing found (if there is one) is not a VarDecl, and point at what it did find?
DiagnoseEmptyLookup should handle the "nothing found" case.
================
Comment at: lib/Sema/SemaOpenMP.cpp:104
@@ +103,3 @@
+ // A threadprivate directive for static block-scope variables must appear
+ // in the scope of the variable and not in a nested scope.
+ if (!isDeclInScope(ND, getCurLexicalContext(), CurScope)) {
----------------
So all of these paragraphs are essentially one check. That's handy, but this formatting makes it read more like we're missing checks. How about eliminating the whitespace between comments and indenting the actual parts that are excerpted (which better matches what we do everywhere else in Clang anyway). For example:
// OpenMP [2.9.2, Restrictions, C/C++, p.2]
// A threadprivate directive for file-scope variables must appear outside
// any definition or declaration.
// OpenMP [2.9.2, Restrictions, C/C++, p.3]
// 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.
================
Comment at: lib/Sema/SemaOpenMP.cpp:124
@@ +123,3 @@
+ if (RequireCompleteType(I->getLoc(), VD->getType(),
+ diag::err_incomplete_type)) {
+ continue;
----------------
I suggest customizing the error diagnostic for the incomplete-type case.
================
Comment at: lib/Sema/SemaOpenMP.cpp:142
@@ +141,3 @@
+ getOpenMPDirectiveName(OMPD_threadprivate) <<
+ VD->getType().getUnqualifiedType();
+ continue;
----------------
What is this trying to check? The restriction here is saying that the address of a thread-private variable is not address-constant. That affects expressions using the variable, not the variable itself. See, e.g., VarDecl::isUsableInConstantExpressions.
================
Comment at: lib/Sema/SemaOpenMP.cpp:146
@@ +145,3 @@
+
+ // Check if threadspecified is set
+ if (VD->isThreadSpecified()) {
----------------
There's a reference for this, is there not?
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:309
@@ -308,2 +308,3 @@
Var->setConstexpr(D->isConstexpr());
+ Var->setOMPThreadprivate(D->isOMPThreadprivate());
----------------
I don't think instantiation should propagate the threadprivate bit directly. Rather, when one instantiates the OpenMPThreadPrivateDecl, it will perform the semantic analysis again (since we'll have concrete types) and set the threadprivate bit. Obviously, Sema::ActOnOpenMPThreadPrivate will need to be split into n ActOn and a Check method, as we do for most other semantic analysis.
================
Comment at: include/clang/AST/Decl.h:773
@@ -772,1 +772,3 @@
+ class OpenMPVarDeclBitfields {
+ friend class VarDecl;
----------------
Why is this a separate set of bitfields? What indicates when we should be using OpenMPVarDeclBitfields rather than VarDeclBitfields?
If this is to be a bit, it should be OpenMPThreadPrivate bit on VarDeclBitfields.
================
Comment at: include/clang/AST/Decl.h:780
@@ +779,3 @@
+ /// \brief is the variable threadprivate
+ unsigned Threadprivate : 1;
+ };
----------------
This is bloating VarDecls by an extra 4 bytes for a single bit of storage. That doesn't make sense; see above.
================
Comment at: include/clang/AST/Decl.h:883
@@ -867,1 +882,3 @@
+ }
+
/// hasLocalStorage - Returns true if a variable with function scope
----------------
Why is this a bit, rather than a storage class? If it's a storage class, then we're forced to think about and make the right decision whether something is querying the storage class. If it's a bit, it's easily forgotten, e.g., the missing address-constant logic.
================
Comment at: include/clang/AST/DeclOpenMP.h:33
@@ +32,3 @@
+
+ VarDecl const * const *getVars() const {
+ return reinterpret_cast<VarDecl const * const *>(this + 1);
----------------
Dmitri Gribenko wrote:
> `const VarDecl` is more in line with LLVM style.
An ArrayRef would be a much nicer return value here.
================
Comment at: include/clang/AST/DeclOpenMP.h:24
@@ +23,3 @@
+/// OMPThreadPrivateDecl - This represents #pragma omp threadprivate ...
+/// directive
+class OMPThreadPrivateDecl : public Decl {
----------------
The comment should show how the pragma is used with an actual code snippet. Mentioning that the actual threadprivate-ness of the referenced variables is also recorded on the VarDecl would be nice, too.
================
Comment at: include/clang/AST/DeclOpenMP.h:25
@@ +24,3 @@
+/// directive
+class OMPThreadPrivateDecl : public Decl {
+ size_t NumVars;
----------------
This class is keeping track of the set of VarDecls being made thread-private, but it loses all source-location information about the actual in-source text used to refer to those variables. We'd like to keep that information (e.g., as DeclarationNameInfos), both because it's good for other tools (e.g., finding all references to a variable) and because you're going to need those locations for diagnostics when you implement template instantiation for this decl.
================
Comment at: include/clang/Basic/LangOptions.h:67
@@ -66,2 +66,3 @@
};
+ enum OpenMPMode { OpenMPModeNotSet, OpenMPModeOff, OpenMPModeOn };
----------------
I don't think this should be a three-state mode. The three states you want seem to be:
- Default: Complain if we see OpenMP (presumably because the user forgot to pass -fopenmp)
- On: we have OpenMP support
- Off: we don't have OpenMP support and don't complain
However, the difference between Default and Off is better handled in the diagnostics system. -fopenmp turns on OpenMP. -Wno-source-uses-openmp will turn off the diagnostics if one wants to compile without OpenMP and not see complains about it. See additional comment about this on the diagnostics bits.
================
Comment at: lib/Parse/ParseOpenMP.cpp:42
@@ +41,3 @@
+ SkipUntil(tok::semi);
+ break;
+ }
----------------
Why should having extra tokens cause us to drop the threadprivate directive entirely? It would be better recovery to just continue on.
================
Comment at: lib/Parse/ParseOpenMP.cpp:76
@@ +75,3 @@
+ UnqualifiedId Name;
+
+ // Read tokens while ')' or ';' is not found
----------------
I'd feel more comfortable moving these into the loop, right before the ParseUnqualifiedId, so it's clear that they are meant to be fresh each time we look for a new identifier.
================
Comment at: lib/Parse/ParseOpenMP.cpp:86
@@ +85,3 @@
+ }
+ if (ParseUnqualifiedId(SS, false, false, false, ParsedType(),
+ TemplateKWLoc, Name)) {
----------------
Is a simple-variable-list supposed to allow qualified names?
================
Comment at: lib/Parse/ParseOpenMP.cpp:107
@@ +106,3 @@
+ T.consumeClose();
+ return IsCorrect;
+}
----------------
LLVM convention is to return false for success, true for failure.
Also, why not return success so long as we've managed to parse any names? Then we'll get more semantic analysis done.
================
Comment at: lib/Parse/ParsePragma.cpp:733
@@ +732,3 @@
+ PP.Diag(FirstTok, diag::warn_pragma_omp_ignored);
+ }
+ PP.DiscardUntilEndOfDirective();
----------------
We usually try to avoid such "already warned" bits in Clang, because we don't know (without checking explicitly) whether a call to Diag() will actually emit the warning. For example, if there were a #pragma omp threadprivate in a system header, that warning would be suppressed and the user would never see a warning.
I suggest removing the iswarned/setwarned stuff. There's nothing like a flood of "I'm ignoring this" warnings to get a user's attention.
================
Comment at: lib/Parse/ParsePragma.h:102
@@ -101,1 +101,3 @@
+class PragmaOpenMPHandler : public PragmaHandler {
+ bool Warned;
----------------
This is really two handlers masquerading as one: the real OpenMP handler and the "complain about anything that looks like OpenMP" handler. They should be separate classes.
================
Comment at: lib/Sema/SemaOpenMP.cpp:31
@@ +30,3 @@
+ if (NamedDecl *ND = Candidate.getCorrectionDecl()) {
+ return isa<VarDecl>(ND) &&
+ Actions.isDeclInScope(ND, Actions.getCurLexicalContext(),
----------------
Do you also want to check for static storage duration, or do you want to allow corrections to variables with automatic storage (and, therefore, emit an error on those corrections)? I suggest the former.
================
Comment at: lib/Sema/SemaOpenMP.cpp:156
@@ +155,3 @@
+ }
+ if (!Vars.empty()) {
+ OMPThreadPrivateDecl *OTD = OMPThreadPrivateDecl::Create(Context,
----------------
I suggest flipping this conditional around, to de-nest more code:
if (Vars.empty())
return DeclGroupPtrTy();
// create the thread-private decl.
http://llvm-reviews.chandlerc.com/D356
More information about the cfe-commits
mailing list